RecursionError with v0.10.0

[hanjinliu]

• psygnal version: 0.10.0
• Python version: 3.11.4
• Operating System: Windows 11

Description

I found in a specific case, psygnal<0.10 works fine but recursion error occurs in the emission loop with 0.10.0.

What I Did

This is a simple example that class A has an evented property value and class Linker can link the values of different A instances.

from psygnal import SignalGroup, Signal

class Events(SignalGroup):
    value = Signal(int)

class A:
    def __init__(self):
        self.events = Events()
        self._value = 0

    @property
    def value(self):
        return self._value

    @value.setter
    def value(self, val: int):
        self._value = val
        self.events.value.emit(val)

class Linker:
    def __init__(self, objects: list[A]):
        self._objs = objects
        for obj in objects:
            obj.events.value.connect(self.set_values)
        self._updating = False

    def set_values(self, val: int):
        if self._updating:
            return  # avoid recursion error
        self._updating = True
        try:
            for obj in self._objs:
                obj.value = val
        finally:
            self._updating = False

a0 = A()
a1 = A()

linker = Linker([a0, a1])

a0.value = -1
assert a1.value == -1

RecursionError                            Traceback (most recent call last)
File src\psygnal\_signal.py:1014, in _run_emit_loop()

RecursionError: 

During handling of the above exception, another exception occurred:

RecursionError                            Traceback (most recent call last)
Cell In[1], line 42
     38 a1 = A()
     40 linker = Linker([a0, a1])
---> 42 a0.value = -1
     43 assert a1.value == -1

Cell In[1], line 18, in A.value(self, val)
     15 @value.setter
     16 def value(self, val: int):
     17     self._value = val
---> 18     self.events.value.emit(val)

File src\psygnal\_signal.py:986, in emit()

File src\psygnal\_signal.py:1017, in _run_emit_loop()

RecursionError: RecursionError in __main__.Linker.set_values when emitting signal 'value' with args (-1,)

[tlambert03]

Hi @hanjinliu, sorry you've hit this error.

The main change that occurred in #281 was to ensure that, when a signal is emitted, all registered callbacks are called before the next emission event. This means that if one of the callbacks also triggers an emit event of the same signal (as your value.setter does here) that emission will wait until all callbacks have been called with the first emission. In other words, it's a bread-first callback pattern instead of a depth first callback pattern. (more on that in a moment)

One solution that should work for you in both cases is to not emit unnecessary events in your value.setter if the value hasn't actually changed:

        @value.setter
        def value(self, val: int) -> None:
            before, self._value = self._value, val
            if val != before:
                self.value_changed.emit(val)

---

However, your comment prompted me to go back and check what Qt's default behavior is for calling callbacks. And I realized it does not do a breadth-first callback pattern. Emission of a signal immediately invokes all callbacks, even if it happens in the middle of another emit event. For example:

from qtpy import QtCore

class Emitter(QtCore.QObject):
    sig = QtCore.Signal(int)

def cb1(value: int) -> None:
    print("calling cb1 with", value)
    if value != 2:
        emitter.sig.emit(2)

def cb2(value: int) -> None:
    print("calling cb2 with", value)

app = QtCore.QCoreApplication([])

emitter = Emitter()
emitter.sig.connect(cb1)
emitter.sig.connect(cb2)
emitter.sig.emit(1)

prints

calling cb1 with 1
calling cb1 with 2
calling cb2 with 2
calling cb2 with 1

So, @Czaki, with #281, we actually changed the behavior of psygnal to be unlike Qt, and possibly be more surprising than the previous behavior. Can you weigh in on what prompted your PR in #281, and why you think we should be calling all callbacks first before entering the second emission round?

[tlambert03]

i think (but am not sure) that the motivation came from some behavior you were observing with EventedSet in napari, is that correct? Is it possible that that specific issue could have been solved without changing the callback order of nested emission events?

[tlambert03]

as a slightly deeper example:

from qtpy import QtCore

class Emitter(QtCore.QObject):
    sig = QtCore.Signal(int)

def cb1(value: int) -> None:
    print("calling cb1 with", value)
    if value == 1:
        emitter.sig.emit(2)

def cb2(value: int) -> None:
    print("calling cb2 with", value)
    if value == 2:
        emitter.sig.emit(3)

def cb3(value: int) -> None:
    print("calling cb3 with", value)

app = QtCore.QCoreApplication([])
emitter = Emitter()
emitter.sig.connect(cb1)
emitter.sig.connect(cb2)
emitter.sig.connect(cb3)
emitter.sig.emit(1)

calling cb1 with 1
calling cb1 with 2
calling cb2 with 2
calling cb1 with 3
calling cb2 with 3
calling cb3 with 3
calling cb3 with 2
calling cb2 with 1
calling cb3 with 1

[Czaki]

I could confirm that also for native objects the order is same as you spotted:

from qtpy.QtWidgets import QApplication, QListWidget

app = QApplication([])

w = QListWidget()
w.addItems(["a", "b", "c", "d", "e"])
w.currentRowChanged.connect(lambda x: w.setCurrentRow(2))
w.currentRowChanged.connect(lambda x: print("currentRowChanged", x))

w.show()
w.setCurrentRow(1)

gives:

currentRowChanged 2
currentRowChanged 1

The problem is that in such a situation you cannot depend on values emitted by signal, as the last signal contains different value than the current state in the object, that introduces inconsistency in the system. If you decide to revert this change, then there will be a need to make sender dispatch thread safe.

[tlambert03]

Thanks @Czaki, your example indeed makes it very clear what the problem is with the default pattern. So, while I do think the default behavior should return to how it was, I think we need an option flag. There are three places where I can imagine having this (not mutually exclusive).

# defined on the signal level
class Thing:
    sig = Signal(int, depth_first=False)

t = Thing()

# declared at the connection level 
t.sig.connect(print, depth_first=False)

# declared at emission time
t.sig.emit(1, depth_first=False)

... option 2 (a connection parameter) would perhaps be the weirdest one, with possibly confusing results and a more difficult implementation, but it would be the only one where the receiver gets to specify how they want to be called.

There's many ways we could go with this, and I think it's a worthwhile feature/switch.

Thoughts?

[Czaki]

There is also a need to allow passing such selection to evented model. I prefer declaration in constructor call Signal(int, depth_first=False)

Also qt is not consistent in order of event emmision

from qtpy.QtWidgets import QApplication, QListWidget
from qtpy.QtCore import Qt

app = QApplication([])

w = QListWidget()
w.addItems(["a", "b", "c", "d", "e"])
w.currentRowChanged.connect(lambda x: w.setCurrentRow(2), Qt.QueuedConnection)
w.currentRowChanged.connect(lambda x: print("currentRowChanged", x), Qt.QueuedConnection)

w.show()
w.setCurrentRow(1)
app.exec_()

provides

currentRowChanged 1
currentRowChanged 2

[tlambert03]

right, Qt::ConnectionType does give the connector (not the emitter) some degree of control over the order in which they will be called. so it seems that there is probably at least a user case for both declaration in the signal with Signal(int, depth_first=False), as well as at connection time.

I'll put together a proposal

[tlambert03]

hi @hanjinliu, I just pushed v0.10.1 which should fix the recursion error you're seeing in your original example.

One can now opt-in to the new behavior with

class Events(SignalGroup):
    value = Signal(int, recursion_mode="deferred")

[hanjinliu]

Hi @tlambert03 , thank you for letting me know. Yes, it seems fixed now. It's nice that both types of signals are supported!