FEATURE: add partial (args/kwargs) field to Action?

[tlambert03]

Should we add the ability store args/kwargs (like a partial) in an Action?

    action1 = Action(id="cmd_id", title="title1", callback=cb)
    action2 = Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1})

or should the callback just be the partial. Need to think about pros cons.
first thought: the "pro" of just using callback=partial(cb, a=1) is that it's familiar regular python. The "con" is that it (currently) would require us to create a new command id for every partial variant. Maybe that's not a big deal

[Czaki]

Yes, we should add this. It will allow writing much cleaner code and avoid leaking memory.

[lucyleeow]

We discussed this recently and @Czaki was in favour of this and NOT partial (was there concern on the use of partials in loop/nested functions?).

@tlambert03 are you happy to take a PR to add kwarg field to Action ?

[tlambert03]

I suspect the concert is strong refs inside the partial, is that true @Czaki?

If so, it implies that the "proper" solution here would need to consider or reimplement all of the weak referencing stuff in psygnal.

@lucyleeow i would definitely appreciate and consider a PR. Id say let's start with the simple/direct approach, not worrying about weakrefs, and see how it generally looks and feels

[Czaki]

I suspect the concert is strong refs inside the partial, is that true @Czaki?

Strong ref, harder debugging, less control. Promoting of bad patterns (not all callbacks are as good, prepared for partial like psygnal).

If so, it implies that the "proper" solution here would need to consider or reimplement all of the weak referencing stuff in psygnal.

I think that we may start with a simple dict, but limit usage to basic types like str, int, float

And I'm not sure if such advanced things like in psygnal are required. As actions are global, how we expect to hardcode short living objects bind to Action.

Or is there an obvious use case for short living actions?

[tlambert03]

Or is there an obvious use case for short living actions?

Nope I'm happy to consider these as long lived objects and just use a simple dict

[lucyleeow]

Revisiting things, could someone clarify, if we are limiting kwargs to basic types, how would this fix the strong refs inside partial problem?

i.e., would passing an int via partial(cb, a=1) be better than Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1}) ?

Or is the improvement here in regards to action kwargs being neater and the code thus easier to debug? Also I am not clear what you mean by partials providing "less control" @Czaki?

Also if we were to implement action kwargs, would we use a partial e.g., partial(self.resolved_callback, **kwargs) here:

app-model/src/app_model/registries/_commands_reg.py

Line 76 in e8fb729

return self._injection_store.inject(self.resolved_callback, processors=True)

or would you pass the kwargs when we execute the callback in Store.inject in in-n-out ?

Thanks

cc @tlambert03

[Czaki]

Revisiting things, could someone clarify, if we are limiting kwargs to basic types, how would this fix the strong refs inside partial problem?

Because we do not worry about keeping reference to a string or float object. We worry about keeping reference to big object's live viewer or big numpy array.

i.e., would passing an int via partial(cb, a=1) be better than Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1}) ?

I, personally, prefer kwargs as an action parameter, as it makes debugging easier. There will be no need to deal with inspecting partial.

Also, I expect that people are looking to napari codebase for inspiration. And they may not notice subtle difference and use partial/lambdas as signal callback, that may lead to storing hard references to problematic objects and even segfaults.

or would you pass the kwargs when we execute the callback in Store.inject in in-n-out ?

I think that we should update in-n-out and pass kwargs directly.

[lucyleeow]

To clarify, if we were to expand kwargs to take complex objects (e.g., window/widgets) we would pass them safely: use a weak ref when passing to Action and before passing to the action callback we would unpack the weak ref?

cc @Czaki

[Czaki]

To clarify, if we were to expand kwargs to take complex objects (e.g., window/widgets) we would pass them safely: use a weak ref when passing to Action and before passing to the action callback we would unpack the weak ref?

We expect to not pass complex objects. We expect to pass int, str, Enum, float etc.
We expect that actions are created on import time, and in such situation, the complex object should not be created.

[lucyleeow]

Ah okay there is never an intention of passing more complex objects.

[tlambert03]

... and if there is never an intention of passing more complex arguments, can you remind me again why

Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1})

is so much better than

Action(id="cmd_id2", title="title2", callback=partial(cb, a=1))

? It kinda seems like added code and complexity here for something that already has an acceptable solution

[Czaki]

is so much better than

Action(id="cmd_id2", title="title2", callback=partial(cb, a=1))

? It kinda seems like added code and complexity here for something that already has an acceptable solution

Napari codebase is a place where people look for inspiration of how to write a code. Connecting partial to signal is a bad pattern that may lead to multiple problems (especially when writing tests).

It is also more readable when going through stacktrace when using debugger.

[tlambert03]

Connecting partial to signal is a bad pattern that may lead to multiple problems (especially when writing tests).

not when all of your arguments are simple. We are far removed here from a simple connection to a signal. In fact, this isn't a direct connection to a signal. it's a connection to an in-n-out function, (whether you connect it to a signal or not is independent of app-model).

Napari codebase is a place where people look for inspiration of how to write a code.

Leaving aside for a moment that this is not the napari repo, that argument is so vague and relatively self-important without much justification that it adds nearly nothing to the motivation for this feature on way or the other. A) it presumes that Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1}) is prima-facie better code (which, it is not, without knowing much more about the situation B) it assumes that someone looking through the codebase and seeing Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1}) has any clue whatsoever that this is somehow avoiding using a partial because somewhere else in the codebase that action might end up being hooked up to a signal somewhere.

I'm looking for substantive discussion here, with concrete issues and proposals. Something more than a one or two line comment that just states as fact that your desired outcome is better.

It is also more readable when going through stacktrace when using debugger.

is it? that kinda entirely depends on how the kwargs feature is added here and in in-n-out. This statement needs much justification

[Czaki]

not when all of your arguments are simple. We are far removed here from a simple connection to a signal. In fact, this isn't a direct connection to a signal. it's a connection to an in-n-out function, (whether you connect it to a signal or not is independent of app-model).

You know this, I know this,and for example jni makes PR with lambdas connected to signals in examples (link.

B) it assumes that someone looking through the codebase and seeing Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1}) has any clue whatsoever that this is somehow

No. This is to not inspire people to use partial in their code. People who learn how to code to implement own napari plugin will not see the difference between Action callback and Signal connected callback.

Main difference between:

1. Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1})
2. Action(id="cmd_id2", title="title2", callback=partial(cb, a=1))

Is that in the first case the reference is stored in the Action class, that is a better pattern in general.

is it? that kinda entirely depends on how the kwargs feature is added here and in in-n-out. This statement needs much justification

When we use kwargs, there is explicit injection of parameters to function, visible as steep in the debugger. When using partial, the injection happens on creation of Action time, and no one could easily use conditional breakpoint on injection steep without need to check value from partial.
Conditional breakpoint on partial need to base on string representation, that is not optimal. But maybe this is too personal feeling.

[tlambert03]

You know this, I know this,and for example jni makes PR with lambdas connected to signals in examples (napari/napari#6881.

But that's only an issue because it's wrapping a complex object right? In a previous post you said "We expect to not pass complex objects. We expect to pass int, str, Enum, float etc." ... and so I asked "in that case, what's so bad about a partial", to which you responded by showing me code where Juan used a partial to bind a very complex object. Can you see why I'm confused with your position here?

[Czaki]

The general problem is that people without experience will not know that there is a difference between wrapping complex and non-complex object. But, as kwargs approach is not easy to apply to signals, it cannot be reproduced, by non-experienced user, as mistake that lead to problematic situation.

[tlambert03]

perhaps one way we could go about this is for you to make a subclass if Action in napari as an exercise in implementing whatever changes you'd like to see. I would be happy to make any changes necessary on this side for you to be able to experiment with new features with thin subclasses, without having to vendor large parts of app-model in order to accommodate those new features. I think that would be mutually beneficial. It would increase the customizability of app-model, it would let you do whatever you want more easily, and by nature of implementing and testing the changes you want to see to app-model, it would help make more concrete the (currently rather abstract) issues being discussed here.

It's not I don't think there's something potentially interesting here in this issue, but I'm not yet seeing a unambiguous and undeniable benefit or motivation behind it.

[Czaki]

I will be happy with such a solution. But As I check, it will also require changes in Command registry? As it will be easier than changing QAction related class?

[tlambert03]

ok, see #194 which passes control of Action registration to the CommandRegistry. Note that the command registry has always been extendable: you need to pass your own custom subclass when you create your Application instance

[tlambert03]

#194 is released in v0.2.7. So if you'd like to experiment more on the napari side, it should now be easier