`enablement`/`when` typing strictly `Expr` ?

[lucyleeow]

Description

Is the typing for enablement and when strictly only Expr ?

app-model/src/app_model/types/_command_rule.py

Line 66 in 8289d03

enablement: Optional[expressions.Expr] = Field(

I thought the typing would be:

app-model/src/app_model/expressions/_expressions.py

Line 81 in 8289d03

expr : str | bool | Expr

Indeed I see in app-model tests a string is used:

app-model/tests/conftest.py

Line 140 in 8289d03

enablement="allow_undo_redo",

[lucyleeow]

ref: https://github.com/napari/napari/pull/6864/files#r1602701616

[lucyleeow]

@tlambert03 slightly related question, how do you think it should be handled when enablement is a function call, that is too complicated to be made into a supported Expression?

In napari/napari#6864 we've set a context key to be the result of that function call, and we do this within show_context_menu which I think is connected to aboutToShow. I don't think this is the blessed way and it seems hacky?

Edit: I've remembered that we've talked about this here: napari/napari#6643 (comment) and it was suggested to calculate contexts when required, i.e., on about to show but I feel like this wasn't so much the intention of app-model? We floated the idea of invalidating contexts, which marks them for recalculation on about to show. Anyway, keen to hear your thoughts.

cc @DragaDoncila

[tlambert03]

I commented in napari/napari#6864 (comment)

Point is: the type annotations declares what it will be after pydantic performs type coercion. If you give a string, it will turn it into an Expr. If you want to avoid type errors in mypy, you can add the pydantic mypy plugin: https://docs.pydantic.dev/latest/integrations/mypy/

[lucyleeow]

Ah right, good to know. Will close but if you have thoughts on how best to handle function call 'expressions', please let me know!

[tlambert03]

@tlambert03 slightly related question, how do you think it should be handled when enablement is a function call, that is too complicated to be made into a supported Expression? In napari/napari#6864 we've set a context key to be the result of that function call, and we do this within show_context_menu which I think is connected to aboutToShow. I don't think this is the blessed way and it seems hacky?

I think "too complicated to be made into a supported Expression" is probably the thing to dig into here.

First, enablement is always a function call in a sense. Specifically, enablement is the result of evaluating the expression, substituting in the values of the context for variable names in the expression:

>>> expression = 'x + 7 < z'
>>> eval(expression, {'x': 5, 'z': 15})
True

I think the key question here is "when do we determine what the values are for the context?

in the relevant show_context_menu method of that PR:

        ctx = get_context(layer_list)
        ctx['valid_spatial_json_clipboard'] = is_valid_spatial_in_clipboard()
        self._context_menu.update_from_context(ctx)

you're essentially just "lazily" computing the value of one of the keys in the context... and you're doing it every time the expression is evaluated. In our simplified example above, it would be like

eval(expression, {'x': 5, 'z': (lambda: 10 + 5)()})

As you can probably intuit from that reduced example there, if the value of z there (10+5) is not changing that often, then we're unnecessarily computing something each time we want to know whether 'x + 7 < z' is True or False. That's why the preferred pattern is to flip that around and update the value of the context once whenever the value of z changes (and, if necessary, use an evented context with a Context.changed signal to let dependent expressions re-evaluate themselves... but that's not the case here with the layer list context menu because it's not visible by default).

in your example: is_valid_spatial_in_clipboard() is calling _get_spatial_from_clipboard() which then deserializes the value of QApplication.clipboard().mimeData(), and this is going to happen every time the layer list context menu is shown (regardless of whether or not the clipboard value has changed).

An alternate pattern would be to connect a callback to QClipboard.changed that updates the context (once) with the value of is_valid_spatial_in_clipboard(). Then, you wouldn't need to patch ctx['valid_spatial_json_clipboard'] = is_valid_spatial_in_clipboard() every time you show the menu.

Does that help? Are there other things that made you suggest this was too complicated to be made into a supported expression?

---

Side note: this does get me thinking about a possible feature, which would be to support lazy evaluation of context values that are expressed as functions. In other words, it would let you set the value of valid_spatial_json_clipboard in your context to the actual function is_valid_spatial_in_clipboard. Then, when the expression is evaluated here, we would check whether any of the values in the expression are callable, and call them.

We don't want to call every function in the context every time any expression is evaluated though. So we would need a way to determine which of the values in the context were needed to evaluate the current expression (i.e. "what are the variables in my expression and are any of them associated with function values"). That would induce additional overhead and make evaluating expressions slower... so i suspect it's not worth it unless we can figure out an eager way to know ahead of time (not at evaluation time) whether any of the context values needed for this expression might be functional.

[DragaDoncila]

@tlambert03 thanks as always for the detail. I think I understand now how the is_valid_spatial_in_clipboard expression could be "flipped" so that rather than updating when we show the menu, we update when we change the clipboard. And I think it makes sense, because you're right that on average we'd show the menu more often than we change the clipboard.

My question then becomes, what if the event that you'd be connecting to, to update the context, happens very frequently? See for example here, where we realized that some of the context menu items need to be updated when the layer attributes change, not just when the layerlist changes. In this case, let's say the enablement depends on something to do with layer.data. Then, if we update the context every time layer.data changes, we've got the opposite of the situation you descirbed right? That is, the event that's changing the context is happening more often than the event that requires the updated context.

Is this a situation where you'd update on aboutToShow, because right clicking happens way less often than data changing? Is this a "bad code smell" that we need to make these updates at all on layer attribute changes?

Side note: this does get me thinking about a possible feature, which would be to support lazy evaluation of context values that are expressed as functions. In other words, it would let you set the value of valid_spatial_json_clipboard in your context to the actual function is_valid_spatial_in_clipboard.

This idea seems like it might help with the situation I described right? If we had a function that checks the status of e.g. active layer attributes on the layerlist, then this could be evaluated as an expression and we'd be home free...?

[lucyleeow]

I think "too complicated to be made into a supported Expression" is probably the thing to dig into here.

I think the key question here is "when do we determine what the values are for the context?

You're spot on. I guess I was thinking of avoiding having to make a new context key. The idea of a function call being run when determining enablement status is appealing.

And I think it makes sense, because you're right that on average we'd show the menu more often than we change the clipboard.

I think this is the essence of the difference in opinion, see: napari/napari#6643 (comment). I think we can agree at least that in some cases menu clicks are rarer and in others context changing is rarer.

In the case when it is more desirable to update on menu click, is it bad code smell to update context keys on aboutToShow? I feel like this isn't the idea of context keys in app-model. It feels like it was designed for the updating of context keys to happen via (context related) events, or maybe I am misguided?

[tlambert03]

It feels like it was designed for the updating of context keys to happen via events, or maybe I am misguided?

just want to first reiterate that app-model is very much still "in-progress", and there's plenty of room for redesign and reconfiguration as we come across situations that don't fit nicely into existing patterns :)

on a higher level here, there has always been a bit of a tension between the whole concept of a context with keys and existing application "state". Something that I'm not fully settled/certain about is "where does the state go". Taken to the extreme, the context could begin to balloon into a complete copy of the model state, which really wasn't my original intention. The original intention of a context was to provide publicly shareable string names (context keys) to values that both the application and third party plugins might need to reference when defining a condition (expression) wherein something should happen. So, it's true in a sense that one should consider carefully whether something demands the creation of a new context key. (however, i think napari is far from having too many keys; that's more of an organizational consideration than a performance consideration)

@DragaDoncila: what if the event that you'd be connecting to, to update the context, happens very frequently?

In these cases, I agree with the statement that @Czaki made in napari/napari#6643 (comment):

In general, we should only invalidate cache on event, recalculation of cache state should happen only on cache access.

and as @lucyleeow said

I think we can agree at least that in some cases menu clicks are rarer and in others context changing is rarer.

absolutely.

All told, i think something like callable context values in #198 does have a roll to play here. Broadly speaking:

1. for cases where the value of a context-key is relatively cheap to compute, does not change terribly often, and/or where many potential "listeners" will need to update themselves in response to the change, the current pattern of directly updating the context is appropriate. This is the current general pattern so I won't give an example.
2. for cases where where the value of a context key is expensive to compute, does potentially change more often that it needs to be accessed, and/or when very few expressions might be paying attention to that value, then it probably makes more sense to have a callable that lazily returns the value. importantly that second pattern is also compatible with a caching strategy: you just need to invalidate the cache on the event (rather than fully compute the value). Here's an example:

import random
import time
from functools import lru_cache

# create a function that can return a value that is expensive to compute when requested
@lru_cache(maxsize=None)
def expensive_value() -> int:
    print("Computing expensive value")
    time.sleep(1)
    return random.randint(0, 100)


# assign the function itself to a context key
my_context = {"expensive_value": expensive_value}


# Define an expression that uses the expensive value
from app_model.expressions import parse_expression

expression = parse_expression("expensive_value")


# Evaluate the expression multiple times
for _i in range(3):
    print(expression.eval_callable_context(my_context))


# invalidate the cache (but don't call the value) on some event
# (such as a clipboard change: QClipboard().changed.connect(expensive_value.cache_clear)
expensive_value.cache_clear()

# now the value will be recomputed the next time it is requested
# when evaluating an expression that requires it
for _i in range(3):
    print(expression.eval_callable_context(my_context))

prints

Computing expensive value
89
89
89
Computing expensive value
15
15
15