feat: emit the old value as second argument in Signals from SignalGroupDescriptor (evented dataclass)

[getzze]

I'm using SignalGroupDescriptor (or evented) with an attrs dataclass and I would like the change signal to emit the old value alongside the new value when mutated.
It's a minimal change to allow that, in _evented_decorator._changes_emitted method __exit__, just change:
self.signal.emit(new) --> self.signal.emit(new, self._prev)

I put the old value in second place because it should allow to connect to slots defined with only the new value as argument.

However, in order to not be a disturbing change, it would be better if it is opt-in.

That is this PR. It is working, but not with subclassing.
Any idea?

import numpy as np
from psygnal import evented, Signal, EmissionInfo
from attrs import asdict, define, make_class, Factory, field, cmp_using

@evented(emit_old_value=True)
@define
class Person:
    name: str
    age: int = 0
    image: np.ndarray = field(eq=cmp_using(eq=np.array_equal), factory=lambda: np.array([]))

@evented(emit_old_value=False)
@define
class Dog:
    age: int

@evented(emit_old_value=False)
@define
class SuperWoman(Person):
    prize: bool = False

john = Person(name="John", age=30)
doug = Dog(age=2)
bridget = SuperWoman(name="Bridget", prize=True, age=None)

@john.events.connect
def on_any_change(info: EmissionInfo):
    print(f"field {info.signal.name!r} changed to {info.args}")

@doug.events.connect
def on_any_change(info: EmissionInfo):
    print(f"field {info.signal.name!r} changed to {info.args}")

@bridget.events.connect
def on_any_change(info: EmissionInfo):
    print(f"field {info.signal.name!r} changed to {info.args}")

# Emit old value
john.age = 21   # Works -> field 'age' changed to (21, 30)
# Do not emit old value
doug.age = 3   # Works -> field 'age' changed to (3,)

# Subclass does not emit old value (but superclass does)
bridget.age = 23  # NOT WORKING (arguably) -> field 'age' changed to (23, None)
bridget.prize = False  # NOT WORKING -> field 'prize' changed to (False, True)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (59472ca) 99.89% compared to head (c8881b2) 99.89%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #257   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          22       22           
  Lines        1908     1908           
=======================================
  Hits         1906     1906           
  Misses          2        2           

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

[codspeed-hq]

CodSpeed Performance Report

Merging #257 will degrade performances by 10.44%

_{Comparing getzze:emit-old-value (c8881b2) with main (59472ca)}

Summary

❌ 1 (👁 1) regressions
✅ 65 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	getzze:emit-old-value	Change
👁	test_create_signal	125.2 µs	139.8 µs	-10.44%

[tlambert03]

I put the old value in second place because it should allow to connect to slots defined with only the new value as argument.
However, in order to not be a disturbing change, it would be better if it is opt-in.

I actually think it should be fine to simply add the second argument. psygnal works fine if the callback takes less (but not more) than the number of arguments in the signal. So, it's always a non-breaking change to pass more arguments to a signal...
The one caveat is that if someone is connecting to the full SignalGroup, and expecting EmissionInfo.args to be a specific length, then they will have an index error. But I think that's probably a very rare occurrence at the moment (we could github search for it) and well worth avoiding the ugliness of an emit_old_value opt-in

can you try reducing this PR down to just the change self.signal.emit(new, self._prev) and seeing if it works for you? While also keeping all of the existing tests passing?

[getzze]

Well, it's true. I will keep it to a minimum then and test it. Thanks

…

On 31 January 2024 18:50:17 GMT+00:00, Talley Lambert ***@***.***> wrote: > I put the old value in second place because it should allow to connect to slots defined with only the new value as argument. > However, in order to not be a disturbing change, it would be better if it is opt-in. I actually think it _should_ be find to simply add the second argument. psygnal works fine if the callback takes less (but not more) than the number of arguments in the signal. So, it's always a non-breaking change to pass more arguments to a signal. can you try reducing this PR down to just the change `self.signal.emit(new, self._prev)` and seeing if it works for you? While also keeping all of the existing tests passing? -- Reply to this email directly or view it on GitHub: #257 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[getzze]

Done and I corrected the tests.

[tlambert03]

thanks! One more change I think we should make:

on line 143 in _group_descriptor.py, in the function _build_dataclass_signal_group can you change the following line

        signals[name] = Signal(object if type_ is None else type_)

to this:

        field_type = object if type_ is None else type_
        signals[name] = Signal(field_type, field_type)

That is where we actually construct all of the Signal instances based on the dataclass to declare the signature of the signal emission, and since we're now emitting two values, we should update that.

want to test out a couple other things locally, but will merge soon. Thanks a lot for your PR!

[tlambert03]

oh, and also it looks like this does break something downstream in magicgui (which depends on psygnal). glad I run those tests here :) don't feel like you have to figure that one out, i'll also have a look into that.

[tlambert03]

ok, the issue there is that magicgui makes use of Signal.connect_setattr

which works fine for a single-argument signal, like this:

>>> class Emitter:
...     sig = Signal(int)
...
>>> class SomeObj:
...     x = 1
...
>>> e = Emitter()
>>> my_obj = SomeObj()
>>> e.sig.connect_setattr(my_obj, 'x')
>>> e.sig.emit(5)
>>> assert my_obj.x == 5

However, (and I think this was probably a poor choice on my part), if the signal emits multiple arguments, it bundles them into a tuple:

>>> class Emitter:
...     sig = Signal(int, int)
...
>>> class SomeObj:
...     x = 1
...
>>> e = Emitter()
>>> my_obj = SomeObj()
>>> e.sig.connect_setattr(my_obj, 'x')
>>> e.sig.emit(5, 2)
>>> assert my_obj.x == (5, 2)

and that turns out to be a breaking change here... since the behavior of connect_setattr specifically changes when going from a single value to multi-value signal. I'm sorry, that's totally my bad. Let me ponder that one a little bit. I'd very much like to avoid adding an emit_old_value argument.

[getzze]

You can force to only accept one argument with e.sig.connect_setattr(my_obj, 'x', maxargs=1).
Maybe it could be inforced in the connect_setattr method, it makes sense that it accepts only one argument by default.

Actually, using max_args=1 when defining a signal.connect callback is the change that people would have to make to ensure the previous behavior is preserved.

>>> class Emitter:
...     sig = Signal(int, int)
...
>>> class SomeObj:
...     x = 1
...
>>> e = Emitter()
>>> my_obj = SomeObj()
>>> e.sig.connect_setattr(my_obj, 'x', maxargs=1)
>>> e.sig.emit(5, 2)
>>> assert my_obj.x == 5

[getzze]

connect_setitem would have the same issue. But again it makes sense to change the default value of the maxargs argument to 1.

[tlambert03]

yep, I agree, that does make sense, and it would be the easiest fix.
But that too is a breaking change for anyone not explicitly passing the maxargs argument to a signal with multiple arguments, expecting to get all the args, so, I think i would want to put that change behind a FutureWarning. I'm thinking maybe the best approach is to temporarily patch the Signal instance specifically used for the evented dataclasses (so that it uses maxargs=1)... and also add the FutureWarning to the main class, so that we can eventually remove the patched version for dataclasses. it's not pretty, but it's a non-breaking way to undo my mess :) will post tonight

[getzze]

I made a search in github for connect_setitem and connect_setattr and the only matches are in magicgui, microvis (your packages) and napari but it seems it's another implementation.
None of the matches define maxargs, so setting the default maxargs=1 for these two methods is probably a limited breaking change.

[getzze]

But that too is a breaking change for anyone not explicitly passing the maxargs argument to a signal with multiple arguments, expecting to get all the args, so, I think i would want to put that change behind a FutureWarning.

I think people would naturally use connect_setattr and connect_setitem with a signal with 1 argument because it is expected to change only one value. So setting maxargs=1 would in fact make more sense than maxargs=None.

What can be a breaking change is connecting the attribute change signal to a slot with one mandatory argument and one optional. Because now the attribute change signal has 2 values, the second value would be passed to the optional argument, making breaking changes...

[tlambert03]

So setting maxargs=1 would in fact make more sense than maxargs=None.

totally agree with this. it's less about "sensical" right at the immediate moment, and more about the safest way to get us there

I made a search in github for connect_setitem and connect_setattr and the only matches are in magicgui, microvis (your packages) and napari but it seems it's another implementation.

Yep, I also did the search, and agree, it's unlikely to affect anyone. napari is currently using an internal event, but our development has been parallel enough (with core devs in both) that it's worth at least maintaining parity. Anyway, call it an overabundance of caution :)

What can be a breaking change is connecting the attribute change signal to a slot with one mandatory argument and one optional.

Yep, good point!

Anyway, please merge in main now that #258 is in, and then add the change mentioned in #257 (comment) ... and we should be good here

[tlambert03]

thanks a lot @getzze for this PR and for helping me think through the implications of the change. merging now, and will release in the next day or two as v0.10.0