feat: allow callable values in contexts

[tlambert03]

same as https://github.com/pyapp-kit/app-model/pull/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[bot]]

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 :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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 (https://github.com/napari/napari/pull/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 :)

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