feat: Support bubbling up of events from evented children on dataclasses #298

tlambert03 · 2024-03-11T20:52:12Z

picking up from #169 ... starting over since lots has changed

from psygnal import evented
from dataclasses import dataclass, field

@evented
@dataclass
class Foo:
    x: int = 1

@evented(connect_child_events=True)
@dataclass
class Bar:
    foo: Foo = field(default_factory=Foo)


bar = Bar()
bar.events.connect(print)
bar.foo.x = 8

prints

EmissionInfo(
    signal=<SignalRelay on Foo(x=8)>,
    args=(
        EmissionInfo(
            signal=<_DataclassFieldSignalInstance 'x' on <SignalGroup 'FooSignalGroup' with 1 signals>>,
            args=(8, 1),
            attr_name=None
        ),
    ),
    attr_name='foo'
)

codecov · 2024-03-11T20:53:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6001216) to head (039328a).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #298   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         2062      2103   +41     
=========================================
+ Hits          2062      2103   +41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codspeed-hq · 2024-03-11T20:54:31Z

CodSpeed Performance Report

Merging #298 will not alter performance

_{Comparing tlambert03:nested2 (039328a) with main (6001216)}

Summary

✅ 66 untouched benchmarks

tlambert03 · 2024-03-12T01:17:48Z

@brisvag, you expressed interest in this a while back. Not sure if it's still something you care about, but have a look at the API above if you're inclined.

@Czaki, if you want to review as well, it's welcome.

@ndxmrb as well, since you recently expressed interest, let me know if it would work for you.

brisvag · 2024-03-12T10:00:04Z

Hey! Excited to see this picked up again :)

I did a quick-and-dirty test with old code from napari/napari#4474. Maybe I converted things wrong, but I don't think it's working with containers a the moment.

I wasn't sure how to use the EventedModel (if it's possible) with nested eventes, so I switched to dataclasses like in your example.

from dataclasses import dataclass, field
from psygnal import evented
from psygnal.containers import EventedList, EventedDict, EventedSet

@evented
@dataclass
class A:
    x: int = 1

@evented(connect_child_events=True)
@dataclass
class M:
    a: EventedList = field(default_factory=lambda: EventedList([1, 2, 3]))
    b: EventedDict = field(default_factory=lambda: EventedDict({'a': 1, 'b': 2}))
    c: EventedSet = field(default_factory=lambda: EventedSet({'x', 'y', 'z'}))
    d: A = field(default_factory=A)

m = M()
m.events.connect(print)

m.a[1] = 12
m.a = [0, 2]
m.b['a'] = 3
m.b = {'x': 11}
m.c.add('w')
m.c = {1}
m.d.x = 12
m.d = {'x': 1}

I would expect 8 events, but I only get 5:

EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'a' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=([0, 2], EventedList([1, 12, 3])),
    attr_name=None
)
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'b' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({'x': 11}, EventedDict({'a': 3, 'b': 2})),
    attr_name=None
)
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'c' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({1}, EventedSet({'w', 'z', 'x', 'y'})),
    attr_name=None
)
EmissionInfo(
    signal=<SignalRelay on A(x=12)>,
    args=(EmissionInfo(signal=<_DataclassFieldSignalInstance 'x' on <SignalGroup 'ASignalGroup' with 1 signals>>, args=(12, 1), attr_name=None),),
    attr_name='d'
)
EmissionInfo(
	signal=<_DataclassFieldSignalInstance 'd' on <SignalGroup 'MSignalGroup' with 4 signals>>,
	args=({'x': 1}, A(x=12)),
	attr_name=None
)

tlambert03 · 2024-03-12T11:33:27Z

K thanks. This was mostly focusing on the models themselves... I'll go back and check all the evented containers too. (We do eventually want them to work)

tlambert03 · 2024-03-12T13:54:18Z

ok @brisvag, evented containers should be working now. Added a test at test_nested_containers.py. Note also that EmissionInfo objects now have a flatten() method, so your example, updated, would be:

from dataclasses import dataclass, field

from rich import print

from psygnal import evented
from psygnal.containers import EventedDict, EventedList, EventedSet


@evented
@dataclass
class A:
    x: int = 1


@evented(connect_child_events=True)
@dataclass
class M:
    a: EventedList = field(default_factory=lambda: EventedList([1, 2, 3]))
    b: EventedDict = field(default_factory=lambda: EventedDict({"a": 1, "b": 2}))
    c: EventedSet = field(default_factory=lambda: EventedSet({"x", "y", "z"}))
    d: A = field(default_factory=A)


m = M()
m.events.connect(lambda e: print(e.flatten()))

m.a[1] = 12
m.a = [0, 2]
m.b["a"] = 3
m.b = {"x": 11}
m.c.add("w")
m.c = {1}
m.d.x = 12
m.d = {"x": 1}

prints

EmissionInfo(
    signal=<SignalInstance 'changed' on <SignalGroup 'ListEvents' with 9 signals>>,
    args=(1, 2, 12),
    loc=('a', 'changed')
)
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'a' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=([0, 2], EventedList([1, 12, 3])),
    loc=('a',)
)
EmissionInfo(
    signal=<SignalInstance 'changing' on <SignalGroup 'DictEvents' with 6 signals>>,
    args=('a',),
    loc=('b', 'changing')
)
EmissionInfo(
    signal=<SignalInstance 'changed' on <SignalGroup 'DictEvents' with 6 signals>>,
    args=('a', 1, 3),
    loc=('b', 'changed')
)
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'b' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({'x': 11}, EventedDict({'a': 3, 'b': 2})),
    loc=('b',)
)
EmissionInfo(
    signal=<SignalInstance 'items_changed' on <SignalGroup 'SetEvents' with 1 signals>>,
    args=(('w',), ()),
    loc=('c', 'items_changed')
)
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'c' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({1}, EventedSet({'z', 'w', 'x', 'y'})),
    loc=('c',)
)
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'x' on <SignalGroup 'ASignalGroup' with 1 signals>>,
    args=(12, 1),
    loc=('d', 'x')
)
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'd' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({'x': 1}, A(x=12)),
    loc=('d',)
)

tlambert03 · 2024-03-12T14:01:24Z

(todo: bubble up from evented containers inside of evented containers)

brisvag · 2024-03-12T16:13:28Z

Great! How does one enable this for EventedModel?

tlambert03 · 2024-03-12T17:16:48Z

How does one enable this for EventedModel?

with patience young padawan 😂 🧘

tlambert03 · 2024-03-12T17:22:26Z

Great! How does one enable this for EventedModel?

a more general and less sarcastic answer:
There's a bit of a divergence in the psygnal codebase at the moment. all the dataclass stuff uses a single SignalGroupDescriptor object:

@dataclass
class MyThing:
    events: ClassVar[SignalGroupDescriptor] = SignalGroupDescriptor()

and that one object is able to inspect the class on which it is defined and create a SignalGroup with events for all the fields.

The psygnal.EventedModel still uses the older napari-style metaclass structure, and therefore has some degree of logic duplication. So, in the short time, implementing this for EventedModels means just going through and doing the corresponding stuff in the EventedModel that I've done here for SignalGroupDescriptor. Ultimately, it would be even better to get EventedModel just using SignalGroupDescriptor itself. That's possible, but we need to maintain support (at least at first) for all the property stuff on evented model. There too, I think it would be good to check in with what pydantic has done in v2 to support properties, and ideally deprecate the features in psygnal's evented model.

anyway, that's the general requirement

ndxmrb · 2024-03-12T19:14:46Z

I was hoping for EventedModel to get this functionality, too. 😋 But it's nice to see this topic driven this fast.

Maybe I didn't fully understand the details of your explanation above, but I guess this is about computed fields?

So providing something like the @computed_field decorator (https://docs.pydantic.dev/latest/api/fields/#pydantic.fields.computed_field), but addinitionally specifying the field_dependencies? Maybe it can be saved into ComputedFieldInfo (https://docs.pydantic.dev/latest/api/fields/#pydantic.fields.ComputedFieldInfo) or a subclass thereof...

tlambert03 · 2024-03-13T01:08:48Z

but I guess this is about computed fields?

Exactly, pydantic v1 didn't allow @property.setter at all. You would get ValueError: object has no field ... when trying to set. psygnal's EventedModel is derived from the work I and others did on napari's EventedModel, including the addition of support for property setters in napari/napari#3711

It was a nice feature to have, but it's also just one more thing to "keep working" as pydantic updates itself. Now that pydantic has added support for property.setter (via their computed properties), I think it would be great to get rid of the extra logic to "allow_property_setters" in psygnal's EventedModel. and (you have it right) simply figure out how to pass on the field_dependencies for events that need to be emitted.

I was hoping for EventedModel to get this functionality, too. 😋

I promise it will! Doing it on dataclasses first is just a good way to set up all the machinery inside of psygnal, and adding it to the EventedModel won't be too difficult. will happen soon

tlambert03 · 2024-03-28T12:28:09Z

mostly a note to self: this is more or less ready to go, but i want to reconsider what we're putting in the loc field, particularly for child events of nested containers, before merging

jni · 2024-06-03T08:55:09Z

@tlambert03 just stumbled on this. It's exciting! Let me know if you want to pair up at some point in case you want someone to talk things out with.

tlambert03 · 2024-06-13T18:23:43Z

@jni, the primary thing that I need to decide here is the API of that loc parameter. it's a little bit overloaded at the moment, with a different meaning for evented fields (as on dataclasses or models) vs signals on an object that don't necessarily represent an addressable value:

for example, here is the same example as above, but with comments putting "what happened" into natural language. Note the slight difference depending on the type of signal (_DataclassFieldSignalInstance signals change the value itself, while container objects change an item within the container)

# item 1 of attribute a changed from 2 to 12
m.a[1] = 12
EmissionInfo(
    signal=<SignalInstance 'changed' on <SignalGroup 'ListEvents' with 9 signals>>,
    args=(1, 2, 12),
    loc=('a', 'changed')
)

# attribute a changed from [1, 12, 3] to [0, 2]
m.a = [0, 2]
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'a' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=([0, 2], EventedList([1, 12, 3])),
    loc=('a',)
)

# item 'a' of attribute b changed from 1 to 3
m.b["a"] = 3
EmissionInfo(
    signal=<SignalInstance 'changing' on <SignalGroup 'DictEvents' with 6 signals>>,
    args=('a',),
    loc=('b', 'changing')
)
EmissionInfo(
    signal=<SignalInstance 'changed' on <SignalGroup 'DictEvents' with 6 signals>>,
    args=('a', 1, 3),
    loc=('b', 'changed')
)

# attribute b changed from {'a': 1, 'b': 2} to {'x': 11}
m.b = {"x": 11}
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'b' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({'x': 11}, EventedDict({'a': 3, 'b': 2})),
    loc=('b',)
)

# attribute c had item 'w' added to the set
m.c.add("w")
EmissionInfo(
    signal=<SignalInstance 'items_changed' on <SignalGroup 'SetEvents' with 1 signals>>,
    args=(('w',), ()),
    loc=('c', 'items_changed')
)

# attribute c was changed from {'z', 'w', 'x', 'y'} to {1}
m.c = {1}
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'c' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({1}, EventedSet({'z', 'w', 'x', 'y'})),
    loc=('c',)
)

# attribute x on attribute d changed from 1 to 12
m.d.x = 12
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'x' on <SignalGroup 'ASignalGroup' with 1 signals>>,
    args=(12, 1),
    loc=('d', 'x')
)

# attribute d changed from  {'x': 12} to {'x': 1}
m.d = {"x": 1}
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'd' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=({'x': 1}, A(x=12)),
    loc=('d',)
)

I want to unify those things somehow so that the consumer doesn't need to do too much conditional logic every time to determine what actually happened. It might require changing the signals on the containers (which would be breaking) or just changing the semantics of loc (which is totally new and added in this PR, and free to change)
In most cases, the non _DataclassFieldSignalInstance signals are the problem. They all currently have the "location" of the changing object in the args emitted by the signal itself. but it feels like that position needs to be moved into loc ... and the name of the signal needs to be taken out

your thoughts on that would be very welcome

ndxmrb · 2024-06-17T08:54:19Z

They all currently have the "location" of the changing object in the args emitted by the signal itself. but it feels like that position needs to be moved into loc ... and the name of the signal needs to be taken out

I felt the same way. Additionally, for example in

# item 1 of attribute a changed from 2 to 12
m.a[1] = 12
EmissionInfo(
    signal=<SignalInstance 'changed' on <SignalGroup 'ListEvents' with 9 signals>>,
    args=(1, 2, 12),
    loc=('a', 'changed')
)

vs

# attribute a changed from 1 to 12
m.a = 12
EmissionInfo(
    signal=<_DataclassFieldSignalInstance 'a' on <SignalGroup 'MSignalGroup' with 4 signals>>,
    args=(1, 12),
    loc=('a',)
)

I need to infer from len(args), that there is an LHS argument involved. Would that still be the plan should it get moved to loc?
Or would that always be an extra tuple which would be empty when no LHS argument is present? Couldn't you have multiple when using foo[0]['x'], too?

Disclaimer: I stiched the second example together from the others above, so it might be wrong...

Also, I saw that on the first of your examples we get args=(1, 2, 12), read: "was 2, now 12". On the second-to-last of your examples we get args=(12, 1), read "now 12, was 1". Was that a copy/paste error, is it on purpose, or should it maybe be unified too between the two signal types?

jni · 2024-06-17T11:20:45Z

src/psygnal/_evented_decorator.py

+        also "evented" (as determined by the `is_evented` function in this module,
+        which returns True if the class has been decorated with `@evented`, or if it
+        has a SignalGroupDescriptor) to the group on the parent object. By default
+        False.


(Which btw I think should be the default... I think it takes a lot of experience with events to even notice that there's no bubbling up by default, let alone grok it. I think most users would expect bubbling to automagically work.)

jni · 2024-06-17T11:31:04Z

src/psygnal/_group.py

    """

    signal: SignalInstance
    args: tuple[Any, ...]
+    loc: str | int | None | tuple[int | str, ...] = None


Looking at the earlier example, which had attr_name here, I find that easier to think about than loc. This is especially true if you add dictionaries into the mix — so now we don't know from the type whether this is an index, a key, or an attribute name. What do you think about having three optionals:

attr

index

key

Of course, now I move on to read about "flatten"... 😅

You could make a lightweight "Loc" object that could contain each of the keys, then you could have a sequence of Loc objects? The nice thing about this option is that you could write an accessor that gets you the right object given the parent and the sequence of locs.

I'm also not super keen on the "loc" name. "from"? "emitted_by"? "child"?

What about "origin"?

I think "loc" would be fine though...

feat: starting on nested events again

45b4f3a

tlambert03 added 2 commits March 11, 2024 17:21

wip

afa4209

finish

7207343

tlambert03 mentioned this pull request Mar 12, 2024

feat: Nested events [wip] #169

Closed

coverage

a8538fd

tlambert03 changed the title ~~feat: nested events - take 2~~ feat: Support bubbling up of events from evented children on dataclasses Mar 12, 2024

tlambert03 added 4 commits March 12, 2024 08:53

rename to loc

5a8b10c

fix tests

456992a

docs

5922aa1

fix containers

e4c4ecd

update test

310a81d

fix bench

c73773f

Merge branch 'main' into nested2

e47b5b4

tlambert03 added 2 commits March 12, 2024 21:10

remove prop

5b55890

Merge branch 'main' into nested2

b149ab0

tlambert03 added 2 commits March 28, 2024 17:43

doc

bc2c67b

Merge branch 'main' into nested2

f5d02d3

jni reviewed Jun 17, 2024

View reviewed changes

Merge branch 'main' into nested2

039328a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support bubbling up of events from evented children on dataclasses #298

feat: Support bubbling up of events from evented children on dataclasses #298

tlambert03 commented Mar 11, 2024 •

edited

Loading

codecov bot commented Mar 11, 2024 •

edited

Loading

codspeed-hq bot commented Mar 11, 2024 •

edited

Loading

tlambert03 commented Mar 12, 2024

brisvag commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

brisvag commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

ndxmrb commented Mar 12, 2024

tlambert03 commented Mar 13, 2024

tlambert03 commented Mar 28, 2024

jni commented Jun 3, 2024

tlambert03 commented Jun 13, 2024 •

edited

Loading

ndxmrb commented Jun 17, 2024 •

edited

Loading

jni Jun 17, 2024

jni Jun 17, 2024

jni Jun 17, 2024

ndxmrb Jul 25, 2024

feat: Support bubbling up of events from evented children on dataclasses #298

Are you sure you want to change the base?

feat: Support bubbling up of events from evented children on dataclasses #298

Conversation

tlambert03 commented Mar 11, 2024 • edited Loading

codecov bot commented Mar 11, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Mar 11, 2024 • edited Loading

CodSpeed Performance Report

Merging #298 will not alter performance

Summary

tlambert03 commented Mar 12, 2024

brisvag commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

brisvag commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

tlambert03 commented Mar 12, 2024

ndxmrb commented Mar 12, 2024

tlambert03 commented Mar 13, 2024

tlambert03 commented Mar 28, 2024

jni commented Jun 3, 2024

tlambert03 commented Jun 13, 2024 • edited Loading

ndxmrb commented Jun 17, 2024 • edited Loading

jni Jun 17, 2024

Choose a reason for hiding this comment

jni Jun 17, 2024

Choose a reason for hiding this comment

jni Jun 17, 2024

Choose a reason for hiding this comment

ndxmrb Jul 25, 2024

Choose a reason for hiding this comment

tlambert03 commented Mar 11, 2024 •

edited

Loading

codecov bot commented Mar 11, 2024 •

edited

Loading

codspeed-hq bot commented Mar 11, 2024 •

edited

Loading

tlambert03 commented Jun 13, 2024 •

edited

Loading

ndxmrb commented Jun 17, 2024 •

edited

Loading