feat: allow callable values in contexts

[tlambert03]

same as #198
(creating a new PR from a different branch cause i accidentally had that one on my main)

This implements the idea I was mentioning in #196 , wherein a context value is allowed to be a callable (that takes no arguments).

In [2]: from app_model.expressions import parse_expression

In [3]: expr = parse_expression("x < 10")

In [4]: expr.eval_callable_context({'x': lambda: 42})
Out[4]: False

this would combine nicely with lru_cache, where the value of a context-key is an lru_cache decorated function, and some event just invalidates the cache rather than calculates the value

[codecov]

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.89%. Comparing base (81b245b) to head (a9071f8).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
src/app_model/expressions/_expressions.py	90.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #199      +/-   ##
===========================================
- Coverage   100.00%   99.89%   -0.11%     
===========================================
  Files           31       31              
  Lines         1817     1830      +13     
===========================================
+ Hits          1817     1828      +11     
- Misses           0        2       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lucyleeow]

Finally had some time to look closer at this.

I think I have two main concerns with this

1. As you've addressed:

The downside would be that it would be evaluated for each expression... so if you know that there are a number of expressions that need that value, it will be slower than evaluating it once for all of them.

Thanks for doing some benchmarking. I would be concerned about how much slower it is for a more complicated function than just lambda: 3 ?

2. It also doesn't address my annoyance with creating a new context (I may have mis-understood here so please correct me):

In the example (napari/napari#6864) the valid_spatial_json_clipboard has been added as a LayerList Context. Although this context key is used in layer actions, it isn't really relevant to LayerList (e.g., the other LayerList context keys are num_layers, num_selected_layers etc..) My concern here is that we either put it with LayerList context keys, where it doesn't really fit (we would also have to make sure it is not documented with them, though this is a 'private' key anyway - we would have to ensure private keys are not auto-doc'd) OR we have to create a new Context and figure out where to put it?

I think the original concern was more having to create a new context key (or whole new context - extra code, extra complexity, though technically probably not a problem) than the updating context manually in aboutToShow (though this does not seem great).

cc @DragaDoncila thoughts?

[tlambert03]

I would be concerned about how much slower it is for a more complicated function than just lambda: 3 ?

this isn't really an app-model consideration, it's more of a end-user consideration: They will either need to:

1. call expensive function, update context, then evaluate any expressions that depend on the corresponding context key
2. wait until the expression needs to be evaluated, then update the context by calling the expensive function.

provided you use an @lru_cache like approach, the total amount of time should be roughly the same. it's just about order of operations. In approach 1, you (greedily) get the result, and the context becomes the cached value. In approach number 2, the context may contain pointers to the functions that can retrieve the value for you when needed... and then any caching is left to the discretion of those functions.

Although this context key is used in layer actions, it isn't really relevant to LayerList ... OR we have to create a new Context and figure out where to put it?

Remember that a context can be any mapping, including ChainMaps. And app_model.expressions.Context (which you use in napari) is a ChainMap, that is scoped at the level of LayerList, but has access to higher level "application" scopes. At the top of that chain is actually the entire napari settings object. For example:

In [1]: import napari

In [2]: v = napari.Viewer()

In [3]: v.layers._ctx['settings.application']
Out[3]:

{
    'first_time': False,
    'ipy_interactive': True,
    'language': 'en',
    'save_window_geometry': True,
    'save_window_state': False,
    'window_position': (486, 142),
    'window_size': (1133, 882),
    'window_maximized': False,
    'window_fullscreen': False,
    'window_state': None,
    'window_statusbar': True,
    'preferences_size': None,
    'gui_notification_level': <NotificationSeverity.INFO: 'info'>,
    'console_notification_level': <NotificationSeverity.NONE: 'none'>,
    'open_history': [],
    'save_history': [],
    'playback_fps': 10,
    'playback_mode': <LoopMode.LOOP: 'loop'>,
    'grid_stride': 1,
    'grid_width': -1,
    'grid_height': -1,
    'confirm_close_window': False,
    'hold_button_delay': 0.5,
    'brush_size_on_mouse_move_modifiers': <BrushSizeOnMouseModifiers.ALT: 'Alt'>,
    'dask': {'enabled': True, 'cache': 17.179869184},
    'new_labels_dtype': <LabelDTypes.uint8: 'uint8'>
}

In [4]: v.layers._ctx['settings.application.playback_mode']
Out[4]: <LoopMode.LOOP: 'loop'>

So, if you feel like a given key doesn't make sense directly in the LayerList context, you're probably right. It probably belongs up higher in the chain.

I know that napari hadn't yet started taking advantage of scoped/chained contexts when I stopped working on it, but that would have been my answer for this particular case of not wanting to add a key to layerlist context

[lucyleeow]

this isn't really an app-model consideration, it's more of a end-user consideration:

Fair point. Looking at the old napari menus, everything was together with the menu definition and you directly deal with the actions which is nice in a sense BUT obviously having a better cohesive structure for all actions via app-model has many benefits.

It probably belongs up higher in the chain.

I'll look into this, thanks!

[tlambert03]

Looking at the old napari menus, everything was together with the menu definition and you directly deal with the actions which is nice in a sense

not immediately sure what you're referring to. Are you referring to the decorator pattern where the action is defined with the function definition? If so, this conversation has been had a few times elsewhere: app-model is fully compatible with that pattern as well (see docs)

is that what you're referring to here? If so, where you declare your actions and how you pair them with callbacks is fully up to you. app-model supports either pattern. doesn't matter to me which you use :)

[DragaDoncila]

@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.

[tlambert03]

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

[tlambert03]

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)

[lucyleeow]

I tried and it works!

[lucyleeow]

not immediately sure what you're referring to.

It's not a problem but I was referring to the debug menu. Here we directly use setEnabled on the menu Action, which I think in app-model we would need to use a context (note this is not a simple toggle) to control enablement. The context will need to added somewhere else (in a separate file).

[lucyleeow]

You've already explained why it's not ideal to be creating new contexts (better than my poorly explained instinct) and adding it to various classes, see your note here:

        # TODO: figure out how to move this context creation bit.
        # Ideally, the app should be aware of the layerlist, but not vice versa.
        # This could probably be done by having the layerlist emit events that
        # the app connects to, then the `_ctx` object would live on the app,
        # (not here)

Edit: responsibility of napari of course, but will need a re-shuffle of current code.

[Czaki]

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

[tlambert03]

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

[DragaDoncila]

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?

[tlambert03]

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

[DragaDoncila]

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.

[DragaDoncila]

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

[Czaki]

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.

[lucyleeow]

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 !

[lucyleeow]

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

[Czaki]

I will try