Closed tlambert03 closed 8 months ago
cc @andy-sweet, @Czaki, @jni ... inasmuch as napari is considering using psygnal for its events, this decision would affect you and I'd welcome your input. Trying to boil it down as briefly as possible:
Are you ok with replacing psygnal.SignalGroup
(the equivalent of vispy/napari's EmitterGroup
) with a more minimal class that does not itself inherit SignalInstance
(the equivalent of vispy/napari's Emitter
). The primary motivation is to avoid name conflicts when one of the signal names chosen by the user conflicts with one of the attribute names on SignalInstance
. The consequence is that connecting to all events within a group would require a special connector (i.e. rather than connecting directly to the signal group with events.connect(on_any_event)
we would add an aggregator like events.agg.connect()
or events.any.connect()
As mentioned above by @getzze, this could be done in a new class (e.g. SignalContainer
), leaving SignalGroup in place to be slowly deprecated. However, I'll note that there are cases (such as evented models/dataclasses) where the user does not directly create the SignalGroup
themselves, so it's not quite so simple as introducing a new opt-in construct (since we'll be creating it at least some of the time)
one more thought, thinking about deprecation:
The most likely "surprise" would be someone trying to connect to everything using events.connect
(on a signal group) and finding that method removed. We could always add a single deprecated attribute on this new class "connect
" to catch this use case and inform the user without direct breakage. And the only time that would break is if they had an event named connect
(which is probably very unlikely)
At this moment my main worry is app-model context that is cached now and require to be connected to some event/signal to update.
At second, I do not understand why SignalGroup
cannot have own implementation of the connect method that will be independent of SignalInstance
implementation? Ex. It will connect to all signals in this group.
At this moment my main worry is app-model context that is cached now and require to be connected to some event/signal to update.
can you please point more directly? I'm not sure I'm seeing the issue
At second, I do not understand why SignalGroup cannot have own implementation of the connect method that will be independent of SignalInstance implementation?
it can, that's what I meant when I said: "We could always add a single deprecated attribute on this new class "connect" to catch this use case and inform the user without direct breakage. And the only time that would break is if they had an event named connect (which is probably very unlikely)"
it can, that's what I meant when I said: "We could always add a single deprecated attribute on this new class "connect" to catch this use case and inform the user without direct breakage.
Why deprecated?
And the only time that would break is if they had an event named connect (which is probably very unlikely)"
As I checked the code, I'm not sure if the connect signal could exit. But maybe I know the wrong order.
can you please point more directly? I'm not sure I'm seeing the issue
This is your instruction from the time of implementing app-model in napari.
https://napari.org/dev/guides/contexts_expressions.html#updating-contexts
Why deprecated?
doesn't have to be, that's what the discussion is for :joy: ... the motivation, as stated above, is to get as many names off of signal group as possible, since it's a likely target for name collision. And, as mentioned, if we left a method connect
on there: "the only time that would break is if they had an event named connect (which is probably very unlikely)". So, in other words, I'm stating that "yes, it's a low likelihood, and needn't necessarily be removed." and seeking opinions on the merits of leaving it vs removing it; please let me know if you have an opinion
This is your instruction from the time of implementing app-model in napari.
yep, I remember that bit... but you said "my main worry is app-model context that is cached now and require to be connected to some event/signal to update." Can you help me out a little bit and be slightly more explicit? For example, tell me what specific cache you're talking about; give me an example of the type of scenario you're talking about. You know, elaborate a little bit; maybe provide a bit of pseudocode, something so I don't have to guess exactly what your worry is
doesn't have to be, that's what the discussion is for š ... the motivation, as stated above, is to get as many names off of signal group as possible, since it's a likely target for name collision. And, as mentioned, if we left a method
connect
on there: "the only time that would break is if they had an event named connect (which is probably very unlikely)". So, in other words, I'm stating that "yes, it's a low likelihood, and needn't necessarily be removed." and seeking opinions on the merits of leaving it vs removing it; please let me know if you have an opinion
So maybe use __getitem__
-> signal_group["connect"](callback)
?
You know, elaborate a little bit; maybe provide a bit of pseudocode, something so I don't have to guess exactly what your worry is
For example this part of code in napari: https://github.com/napari/napari/blob/4f4c063ae5dd79d6d188e201d44b8d57eba71909/napari/_qt/containers/_layer_delegate.py#L307-L318
Where the context needs to be updated each time when the state of layer list is changed. So the need to manually cur rate the list of events to be connected is error-prone.
the need to manually cur rate the list of events to be connected is error-prone.
what need to manually curate the list of events? To be honest, my working hypothesis at the moment is that you haven't really read/understood what the proposal is here, and I'm trying to determine whether that's the case. Are you under the impression that there is a proposal to completely remove the possibility to connect to every event in a signal group? (to be clear, that's not the case. at worst, it will be a slightly different syntax, at best, it will not change at all)
Are you under the impression that there is a proposal to completely remove the possibility to connect to every event in a signal group?
yes. If I'm wrong then I have no problem with this proposal.
I don't have a strong opinion here, other than events.any.connect
is imho much, much better than events.agg.connect
. wtf is agg. š (And why would agg not be a potential name collision, e.g. notify me when the anti-grain geometry changes. š) But regarding the decision points already discussed:
connect
method, that make migrating much easier.connect
.Did I miss any other decision points?
And why would agg not be a potential name collision
the single name would be modifiable with a type hint (see implementation in #269). currently, I'm using events.all
as the default name... but. agg
means "aggregator". a single thing that aggregates all events. I don't think it's that ridiculous, but i do like all/any better
yes, let's add a connect method, that make migrating much easier.
all methods will pass through with a deprecation warning (see #269).
let's not deprecate it for now ā deprecation can happen later if we decide it's an anti-pattern / if many users turn out to want to call an event connect.
I currently do have it deprecated in #269... but we can revisit this. I think it's the better way to go personally, but can pull that back if folks want. Note that connect
is just one of many names on SignalInstance. So it's not a just a single name we need to worry about... There are quite a few public attributes on SignalInstance, the problem is much broader than connect
At any rate, @Czaki's idea to use getitem in cases of name clashes is pretty good
that was already in the proposal, and yes, is being implemented.
I don't think it's that ridiculous,
I was being flippant of course, but I think it takes a fair bit of programming background to leap from agg to aggregator and then from aggregator to ok this is all the child events. But anyway we are in agreement. š
So it's not a just a single name we need to worry about... There are quite a few public attributes on SignalInstance, the problem is much broader than connect
How many of these are really user-facing? I think as a user, connect and disconnect would be the main things I would interact with on a signal group, as well as emitters
which I guess would stay anyway? (As a library, yes, we have work to do, but I'm ok with that.)
How many of these are really user-facing?
there's also .name
, block
, .blocked()
, pause
, resume
, .paused()
, .(dis)connect_setattr
, .emit
etc ... many of these are very valid public user-facing attributes that I use in my projects. In any case, I don't really think we can just pick a few we think users are paying attention to. public is public, and I think we either pass them all through with deprecation or pass them all through without deprecation. (rather than picking a few
Sorry, I missed the discussion here, but the main decisions and actions seem great.
I like SignalGroup
as a mapping/iterable, where __getitem__
is the main part of the interface I'll think about.
Without thinking too hard, I don't love exposing SignalGroup.all
as a property with the SignalRelay
type if it's only there to connect to all the events in the group/container. I would have preferred a connect_all
method that does the same thing. But don't feel too strongly and that would be easy enough to follow up on later if there were a good enough case for it.
if it's only there to connect to all the events in the group/container. I would have preferred a connect_all method that does the same thing.
it's not only there to provide connect
... it needs to provide all of the methods of a signal instance as well, including disconnect, block, blocked, pause, resume, etc... All of those names are currently exposed at the top level group object, so it's a mess. picking one name all
to contain all of that functionality seemed like the best way to provide all the same functionality within a single name
This is meta-issue collecting thoughts and proposals about solving name conflicts in SignalGroups. (see also)
260
261
262
265
The primary problem stems from the fact that
SignalGroup
inherits fromSignalInstance
(which has a fair number of attributes, likename
, etc...)SignalGroup
are determined by the user, and accessible as attributesSignalGroup().name
)As a little history, the pattern of inheritance was originally borrowed/inspired from vispy's
EmitterGroup
(akin toSignalGroup
, which inherits fromEventEmitter
(akin toSignalInstance
). I'll note that vispy simply forbids you to use a name that conflicts. But I don't think that's an acceptable approach for psygnal, particularly if we want to support evented dataclasses (which are almost certainly going to have fields likename
at some point).At the moment, I'm leaning towards a new class entirely, as proposed by @getzze in https://github.com/pyapp-kit/psygnal/issues/262#issuecomment-1936111492 and copied below. This new class would be a plain container with minimal private attributes, preventing name conflicts. The primary thing we lose there by not inheriting from
SignalInstance
is that you can't connect to the group class itself using, for exampleevents.connect
... but that's easily solveable by either adding an aggregating signal (e.g.events.agg.connect()
), or a special method (e.g.events.connect_all()
... risking name conflicts)Originally posted by @getzze in https://github.com/pyapp-kit/psygnal/issues/262#issuecomment-1936111492
I was thinking that
SignalGroup
could be a container class instead.Positive:
__getitem__
, withevents["foo.sig"]
Negative
events.connect
would be longer,events.agg.connect
SignalContainer
, and do not touchSignalGroup
so people could choose to use one or the other.