Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 42 additions & 14 deletions src/app_model/expressions/_expressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,6 @@ class Expr(ast.AST, Generic[T]):
>>> new_expr.eval(dict(v2="hello!", myvar=8))
'hello!'

you can also use keyword arguments. This is *slightly* slower
>>> new_expr.eval(v2="hello!", myvar=4)

serialize
>>> str(new_expr)
'myvar > 5 and v2'
Expand Down Expand Up @@ -184,28 +181,59 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
if type(self).__name__ == "Expr":
raise RuntimeError("Don't instantiate Expr. Use `Expr.parse`")
super().__init__(*args, **kwargs)
self.eval = self.eval_with_callables # type: ignore[method-assign]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this line? Or why declare method below? This will crash inheritance. I do not understand this pattern.

Copy link
Copy Markdown
Member Author

@tlambert03 tlambert03 May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm still not sure that this entire PR is a good pattern, due to the overhead it potentially adds. So for now, I'm making it easy to switch back and forth between the variant that allows callables and the variant that does not allow it, without adding an additional conditional check every time anything is evaluated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm still not sure that this entire PR is a good pattern, due to the overhead it potentially adds.

is that to say you're not sure whether you want to release with this PR merged, or just that you want an easy way to toggle back and forth?

Copy link
Copy Markdown
Member Author

@tlambert03 tlambert03 May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more the former? more like I'm really not sold on this whole pattern. I put it up as an experiment for you all to try, and if you say "yes please I can't live without this feature", then I'll merge it... but otherwise I'd probably not. So, if you want it, then i'm looking for feedback (@Czaki, that includes you) as to whether you want it enabled all the time by default, opt-in, or opt-out. And, if one of the opt-patterns, how would you want to opt in/out since you don't like this pattern of setting the eval() method

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm well, it's not so much that I can't live without it, but it definitely improves our code in a couple of places where we don't want to update context keys based on events because they're too frequent. I generally really like the idea of "lazily evaluated" context keys, because it allows us to maintain the pattern of "reading from the context always gives you the correct value" without us manually going in and populating context keys just before we need them, which I think eventually could be very messy.

In terms of opt-in/opt-out I'd be happy to make it opt-in if you think it's overall too dangerous a pattern to make default. I have no specific preference on how to opt-in though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of right click menu (layer list) ContextMapping may be created each time the right click menu is triggered.

@Czaki how are you seeing this integrate with the existing app-model contexts? Are you imagining the ContextMapping holding the "static" context keys of a given context in its _base_store? I guess for me the question of "how do you update context keys you don't want to connect to events" merits answering with a general solution in app-model, but if it's easier to implement on the napari side, we can go that way. I just don't want a clunky fix that we then have to maintain forever, if this is a real use-case

Copy link
Copy Markdown
Contributor

@Czaki Czaki Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about use it here:

https://github.com/napari/napari/blob/f640234ca73f77305f3ffd8c09b77dff36c8aa60/napari/_qt/containers/_layer_delegate.py#L325-L326

Where functions like is_valid_spatial_in_clipboard could be part of context, so it could be something like:

        ctx = ContextMapping(get_context(layer_list))
        self._context_menu.update_from_context(ctx) 

Where is_valid_spatial_in_clipboard is already part of the context, but as a function, not value.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talking with @DragaDoncila, if you don't think you want to support this in app-model we'll implement this new context in napari and let you know if we run into any concerns.
Feel free to close this PR @tlambert03 !

Copy link
Copy Markdown
Collaborator

@lucyleeow lucyleeow Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Czaki any interest in putting in a PR (in napari) since it was your idea? Happy to have a go if not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try

self._recompile()

def _recompile(self) -> None:
ast.fix_missing_locations(self)
self._code = compile(ast.Expression(body=self), "<Expr>", "eval")
self._names = set(self._iter_names())

def eval(
self, context: Mapping[str, object] | None = None, **ctx_kwargs: object
) -> T:
def eval(self, context: Mapping[str, object] | None = None) -> T:
"""Evaluate this expression with names in `context`.

Parameters
----------
context : Mapping[str, object] | None
Mapping of names to objects to evaluate the expression with.
"""
# will have been replaced in __init__
raise NotImplementedError("This method should have been replaced.")

def eval_no_callables(self, context: Mapping[str, object] | None = None) -> T:
"""Evaluate this expression with names in `context`."""
if context is None:
context = ctx_kwargs
elif ctx_kwargs:
context = {**context, **ctx_kwargs}
context = {}
try:
return eval(self._code, {}, context) # type: ignore
return eval(self._code, {}, context) # type: ignore[no-any-return]
except NameError as e:
miss = {k for k in self._names if k not in context}
raise NameError(
f"Names required to eval this expression are missing: {miss}"
) from e
raise self._missing_names_error(context) from e

def eval_with_callables(self, context: Mapping[str, object] | None = None) -> T:
"""Evaluate this expression with names in `context`, allowing callables."""
if context is None:
return self.eval_no_callables({})

# build a new context, evaluating any callables
# we only want to evaluate the callables if they are needed, so we
# build a new context with only the names in this expression.
ctx = {}
for name in self._names:
if name in context:
ctx[name] = val() if callable(val := context[name]) else val
else:
# early exit if we're missing names
raise self._missing_names_error(context)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tlambert03 I tried playing with stuff in napari and this branch, but I get this error with napari main when I right-click on a layer. Sometimes the error is thrown because name = '', and sometimes the name makes sense but the context passed only contains the app_model_context keys. Not sure if it's something we're doing on our end that's been exposed by this branch, or if it's something on the app-model end.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for trying it out. it's on the app-model end. will write more later

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, try again. I fixed a bug that was introduced in #197 and added a test to prevent regression.

I just tried this branch on napari and it worked:

  1. removed ctx['valid_spatial_json_clipboard'] = is_valid_spatial_in_clipboard() from _layer_delegate.py
  2. added self._selection_ctx['valid_spatial_json_clipboard'] = is_valid_spatial_in_clipboard in layerlist.py (this is probably not the proper place to set that value... but i did it for quick testing.
  3. That works as is, but for better performance, I then decorated is_valid_spatial_in_clipboard() with @functools.cache ... (you would then want to come up with a cache invalidation strategy)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and it works!

return self.eval_no_callables(ctx)

def _missing_names_error(self, context: Mapping[str, object]) -> NameError:
"""More informative error message when names are missing."""
miss = {k for k in self._names if k not in context}
num_keys = len(context)
return NameError(
f"Names required to eval expression '{self}' are missing: {miss}. "
f"Context has {num_keys} keys."
)

@classmethod
def parse(cls, expr: str) -> Expr:
Expand Down
6 changes: 0 additions & 6 deletions tests/test_context/test_expressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,6 @@ def test_safe_eval():
safe_eval("{1,2,3}")


def test_eval_kwargs():
expr = parse_expression("a + b")
assert expr.eval(a=1, b=2) == 3
assert expr.eval({"a": 2}, b=2) == 4


@pytest.mark.parametrize("expr", GOOD_EXPRESSIONS)
def test_hash(expr):
assert isinstance(hash(parse_expression(expr)), int)
4 changes: 3 additions & 1 deletion tests/test_qt/test_qmenu.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ def test_menu(
assert redo_action.isEnabled()

# useful error when we forget a required name
with pytest.raises(NameError, match="Names required to eval this expression"):
with pytest.raises(
NameError, match="Names required to eval expression 'allow_undo_redo'"
):
menu.update_from_context({})

menu._disconnect()
Expand Down