Closed tlambert03 closed 8 months ago
Comparing tlambert03:new-signal-group
(dbbd2fa) with main
(94d0a67)
โ 5 (๐ 5)
regressions
โ
61
untouched benchmarks
Benchmark | main |
tlambert03:new-signal-group |
Change | |
---|---|---|---|---|
๐ | test_dataclass_group_create[attrs] |
2.4 ms | 2.8 ms | -14.79% |
๐ | test_dataclass_group_create[dataclass] |
2.1 ms | 2.4 ms | -14.9% |
๐ | test_dataclass_group_create[msgspec] |
2.4 ms | 2.8 ms | -11.44% |
๐ | test_dataclass_group_create[pydantic] |
2.4 ms | 3.1 ms | -23.5% |
๐ | test_evented_setattr |
37.9 ยตs | 45.2 ยตs | -16.22% |
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
94d0a67
) 100.00% compared to head (dbbd2fa
) 100.00%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@getzze, if you have a moment to take a look, I would appreciate it! This implements your proposal from https://github.com/pyapp-kit/psygnal/pull/262#issuecomment-1936111492
It does not yet add aliases or prefixes as you've done in other PRs (you can do those in another PR if you still want them after this)
one open question about the name all
for the SignalRelay (the one that can be used to connect everything with events.all.connect
). Should we just a clunkier but always guaranteed public name like events.all_psygnals
(either in place of, or in addition to the short name provided via type annotation:
from psygnal import SignalGroup, SignalRelay
class MyGroup(SignalGroup):
all_the_events: SignalRelay
I had a look, but I was waiting for you to "finish" it before trying it. I think having the possibility to "getitem"s is great!
On 13 February 2024 14:45:40 GMT+00:00, Talley Lambert @.***> wrote:
@getzze, if you have a moment to take a look, I would appreciate it! This implements your proposal from https://github.com/pyapp-kit/psygnal/pull/262#issuecomment-1936111492
It does not yet add aliases or prefixes as you've done in other PRs (you can do those in another PR if you still want them after this)
-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/269#issuecomment-1941675893 You are receiving this because you were mentioned.
Message ID: @.***>
thanks! Yep, I think it's approaching stability and ready for review now.
Because we have the dict-like access, I was thinking to dedicate one key to the relay, like events["__all__"]
. What do you think?
On 13 February 2024 16:09:11 GMT+00:00, Talley Lambert @.***> wrote:
one open question about the name
all
for the SignalRelay (the one that can be used to connect everything withevents.all.connect
). Should we just a clunkier but always guaranteed public name likeevents.all_psygnals
?-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/269#issuecomment-1941901423 You are receiving this because you were mentioned.
Message ID: @.***>
I was thinking to dedicate one key to the relay, like
events["__all__"]
. What do you think?
i think i'm (mildly) against it at this point. reasons being:
'__all__'
key should be included in the keys yielded by __iter__
, and i think both including it and excluding it are confusing:
__iter__
, then we should also probably make __len__
return +1 (otherwise len(self) != len(list(self))
... and i think that makes both len
and iter
less useful (i can imagine needing to now add a conditional to check whether the key is the special key or not__iter__
(which would be my preference) then it sorta breaks the Mapping-like pattern, and the only way one would know about it is to "just know about it"... which is not detrimental per se ... but it also doesn't seem all that helpful given that it doesn't add new functionality.__getitem__
, it's harder (though not impossible) to catch in refactoring and stuffYou are right that having a "all" key would make the behavior of iter
inconsistent. I agree with you that we should not have it then.
On 13 February 2024 18:06:02 GMT+00:00, Talley Lambert @.***> wrote:
I was thinking to dedicate one key to the relay, like
events["__all__"]
. What do you think?i think i'm (mildly) against it at this point. reasons being:
- we already have both a private and public way to access that object. so it's doesn't add anything, just creates additional API surface.
- it raises the question of whether the
'__all__'
key should be included in the keys yielded by__iter__
, and i think both including it and excluding it are confusing:
- if we include it in
__iter__
, then we should also probably make__len__
return +1 (otherwiselen(self) != len(list(self))
... and i think that makes bothlen
anditer
less useful (i can imagine needing to now add a conditional to check whether the key is the special key or not- if we don't include it in
__iter__
(which would be my preference) then the only way one would know about it is to "just know about it"... which is not detrimental per se ... but it also doesn't seem all that helpful given that it doesn't add new functionality.- as a string value passed to
__getitem__
, it's harder (though not impossible) to catch in refactoring and stuff-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/269#issuecomment-1942119238 You are receiving this because you were mentioned.
Message ID: @.***>
my inclination is to merge this and release a pre-release candidate that sits for a week or two. @getzze, if you would like time to have a closer/longer look at this PR before merge, just let me know (no hurry at all) and I'll wait on that. @Czaki and @jni, if either of you wanted a final chance to comment on the implementation, here ya go
I tried pushing the potential conflicts and it works very well :)
from dataclasses import dataclass, field
from typing import ClassVar
from psygnal import SignalGroupDescriptor, EmissionInfo, Signal
@dataclass
class Model:
events: ClassVar = SignalGroupDescriptor()
connect: int = 3
name: str = "name"
all: float = 1.0
m = Model()
@m.events.all.connect
def on_any_change(info: EmissionInfo):
print(f"field {info.signal.name!r} changed to {info.args}")
m.events.all # <SignalRelay on Model(connect=3, name='name', all=1.0)>
m.events.connect # <_DataclassFieldSignalInstance 'connect' on <SignalGroup 'ModelSignalGroup' with 3 signals>>
m.events["all"] # <_DataclassFieldSignalInstance 'all' on <SignalGroup 'ModelSignalGroup' with 3 signals>>
m.events.connect(print) # prints: "field 'connect' changed to (<built-in function print>,)"
# this can be strange as it is equivalent to m.events.connect.emit(print)
# but hey it was not even allowed before.
Also, it would be good to define a function that returns the SignalRelay
from a SignalGroup
, so we have an easy public API to get the relay, an API that would not change if the private attributes of SignalGroup
are redefined for instance. It would be better if it was a method of SignalGroup
but the name could confict with signals.
def get_relay(group: SignalGroup) -> SignalRelay:
return group._psygnal_relay
[EDIT] Maybe it also makes sense to have a function to return a dict of signals now that the signals
method is deprecated:
def get_signals(group: SignalGroup) -> Mapping[str, SignalInstance]:
return group._psygnal_instances
Some idea for later (for instance for the nested events): add a method to SignalRelay to merge two SignalRelays:
def update(self, relay: SignalRelay, take_over: bool = True):
self._signals.update(relay._signals)
self._sig_was_blocked.update(relay._sig_was_blocked)
for sig in relay._signals.values():
if take_over:
sig.disconnect(relay._slot_relay)
sig.connect(self._slot_relay, check_nargs=False, check_types=False, unique=True)
Also, it would be good to define a function that returns the
SignalRelay
from aSignalGroup
, so we have an easy public API to get the relay, an API that would not change if the private attributes ofSignalGroup
are redefined for instance. It would be better if it was a method ofSignalGroup
but the name could confict with signals.
yeah, this is what I was getting at in https://github.com/pyapp-kit/psygnal/pull/269#issuecomment-1941901423
I agree, there should be a guaranteed public property or method. my inclination would be an @property
, as the final syntax is nicer, but do you have a preference for a getter method instead?. And the exact name is the main question. Something like .psygnal_relay
is pretty guaranteed to never have a name conflict.
And if we do that, is there perhaps no reason to provide all the logic for letting them override the public name? Maybe one safe public name is all we need
I was proposing functions and not methods to avoid any name conflict. But if the method name starts with psygnal
and is long, it's very unlikely that it will conflict with a dataclass field. And a method is more convenient because of auto-completion.
The good thing about all
is that it's short. Maybe it's not worth putting all the code for allowing changing the all
name though, and just have a long property/method name. If it's documented what the possible conflicts are and there are very few and unlikely names it should be ok.
We can always add possibility to use aliases, which are useful even after this PR, to alias a private field to a public name.
thanks @getzze! yeah I agree. feels tough to make this rather tiny-but-important decision. I might ping @jni for his thoughts on this:
Juan, we all like the idea of events.all.connect
... but all
still has the possibility of name conflict (as you pointed out before). The current PR allows the user to change the name all
to something else if they want to use it for themselves (thereby avoiding the conflict) ... but then we have the problem of a third party wanting to know how to get at the aggregate/relay connector. That leads to a desire to have a permanent public name like events.all_psygnals.connect
or events.get_relay().connect
, which are uglier, to be sure. What do you think? some options:
events.all
by default, but allow the user to pick their own public name, and to hell with anyone else who receives the object :joy: (this is probably a no... since magicgui for example needs to know how to get at the relay)events.all
, and allow user-overrides, and add a permanent safe public name like events.all_psygnals
(name suggestions welcome)events.all
, and if someone has a dataclass with a field named all
, then they must access it using group['all']
. because events.all
will always point to the relay.events.all
, just go with one permanent ugly public name events.all_psygnals
that's unlikely to ever conflict with a dataclass field.I think I like 3 better.
Somebody mentioned the example of pandas
that I think is a good model: you can access ALL the columns by key, like events["name"]
, and by attribute, only the columns with a well-behaved (no space) and non-conflicting name.
On 15 February 2024 21:43:16 GMT+00:00, Talley Lambert @.***> wrote:
thanks @getzze! yeah I agree. feels tough to make this rather tiny-but-important decision. I might ping @jni for his thoughts on this:
Juan, we all like the idea of
events.all.connect
... butall
still has the possibility of name conflict (as you pointed out before). The current PR allows the user to change the nameall
to something else if they want to use it for themselves (thereby avoiding the conflict) ... but then we have the problem of a third party wanting to know how to get at the aggregate/relay connector. That leads to a desire to have a permanent public name likeevents.all_psygnals.connect
orevents.get_relay().connect
, which are uglier, to be sure. What do you think? some options:
- stick with
events.all
by default, but allow the user to pick their own public name, and to hell with anyone else who receives the object :joy: (this is probably a no... since magicgui for example needs to know how to get at the relay)- keep
events.all
, and allow user-overrides, and add a permanent safe public name likeevents.all_psygnals
(name suggestions welcome)- use
events.all
, and if someone has a dataclass with a field namedall
, then they must access it usinggroup['all']
. becauseevents.all
will always point to the relay.- don't use
events.all
, just go with one permanent ugly public nameevents.all_psygnals
that's unlikely to ever conflict with a dataclass field.-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/269#issuecomment-1947387097 You are receiving this because you were mentioned.
Message ID: @.***>
you can access ALL the columns by key, like
events["name"]
, and by attribute, only the columns with a well-behaved (no space) and non-conflicting name.
yep, and combined with the warning at class creation that you stubbed out in https://github.com/pyapp-kit/psygnal/pull/269#pullrequestreview-1882868728 ... i think there will be no surprises
I like 3 also, by a fair margin, so I am happy that we are all in agreement. ๐
implemented in 47bf3ae
ok! this one is going in :)
Thanks all for the helpful discussion. Huge thanks to @getzze for getting this name-conflict discussion started in #260, and for proposing the solution that was ultimately implemented here in https://github.com/pyapp-kit/psygnal/pull/262#issuecomment-1936111492. It's not often that someone comes along and dives deep enough to understand the existing codebase and provides very insightful and productive suggestions. I hope you don't mind that I ran with that idea and implemented it here (it was a big change that I wanted to consider carefully), and I hope that you'll continue contributing improvements :)
this will be released as v0.10.0rc0 for a week or so before release
Ahahah, likewise, it's not every time that developers include contributors, who know less about the project, to make big changes.
I have others PRs to propose ;)
On 16 February 2024 14:54:46 GMT+00:00, Talley Lambert @.***> wrote:
ok! this one is going in :)
Thanks all for the helpful discussion. Huge thanks to @getzze for getting this name-conflict discussion started in #260, and for proposing the solution that was ultimately implemented here in https://github.com/pyapp-kit/psygnal/pull/262#issuecomment-1936111492. It's not often that someone comes along and dives deep enough to understand the existing codebase and provides very insightful and productive suggestions. I hope you don't mind that I ran with that idea and implemented it here (it was a big change that I wanted to consider carefully), and I hope that you'll continue contributing improvements :)
-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/269#issuecomment-1948530828 You are receiving this because you were mentioned.
Message ID: @.***>
This PR implements the idea proposed by @getzze in #267
Notable changes in this PR:
SignalGroup
is no longer aSignalInstance
subclass. All methods that used to be accessible onSignalGroup
(such asconnect
,disconnect
,blocked
, etc...) should now be accessed on the signal relay atgroup.all
.SignalGroup.__getitem__
, in addition to__getattr__
.__iter__
(to list all signal names),__len__
(number of signals),__getitem__
(get a signal). But does not currently subclassabc.Mapping
(so it does not haveitems()
,values()
,keys()
)Deprecations:
connect
, etc) on a SignalGroup is deprecated, (but still works for now). Usinggroup.all
instead.signals
on a group instance (to get a dict of name->signal instance) is deprecated. Instead use a combination of Mapping methods__iter__
and__getitem__
/__getattr__
Breaking:
SignalGroup
no longer inheritsSignalInstance
, the semantics of the__len__
method on a signal group changes; it now returns the number of signal names, not the number of connected slots. This is probably the main breaking change that I can't deprecate.closes #260 closes #267