pyapp-kit / psygnal

Python observer pattern (callback/event system). Modeled after Qt Signals & Slots (but independent of Qt)
https://psygnal.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
84 stars 13 forks source link

refactor: change EmitLoopError message, and mechanism of info gathering #302

Closed tlambert03 closed 7 months ago

tlambert03 commented 7 months ago

This PR avoids the need to hang on to any information about the current callback & args by inspecting the stack after an exception happens. I think it also makes the error a bit easier to read.

With the following error as an example:

from psygnal import SignalInstance

sig = SignalInstance((int,), name="sig")

@sig.connect
def some_func(x: int) -> None:
    print(1 / x)

sig.emit(0)

before this PR:

# ... stack trace
psygnal._exceptions.EmitLoopError:
While emitting signal 'builtins.NoneType.sig', an error occurred in callback '__main__.some_func'.
The args passed to the callback were: (0,)
This is not a bug in psygnal.  See 'ZeroDivisionError' above for details.

after this PR:

# ... stack trace is the same
psygnal.EmitLoopError:

While emitting signal "<SignalInstance 'sig'>", an error occurred in a callback:
  File /Users/talley/dev/self/psygnal/x.py:8, in some_func
  With local variables:
    x = 0

See ZeroDivisionError above for details.

@Czaki, since it does some mildly deep inspection, your eyes on this would be appreciated.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (4ea0184) to head (d8309b2).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #302 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 22 22 Lines 2033 2071 +38 ========================================= + Hits 2033 2071 +38 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codspeed-hq[bot] commented 7 months ago

CodSpeed Performance Report

Merging #302 will not alter performance

Comparing tlambert03:exception-stack (d8309b2) with main (4ea0184)

Summary

✅ 66 untouched benchmarks

tlambert03 commented 7 months ago

side-note: I played a bit with using raise EmitLoopError(...).with_traceback(modified_tb), and I kinda like it, but haven't added it here. With the current PR, the full tb is:

Traceback (most recent call last):
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1045, in _run_emit_loop
    self._run_emit_loop_inner()
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1059, in _run_emit_loop_immediate
    caller.cb(args)
  File "/Users/talley/dev/self/psygnal/src/psygnal/_weak_callback.py", line 357, in cb
    self._f(*self._args, *args, **self._kwargs)
  File "/Users/talley/dev/self/psygnal/x.py", line 8, in some_func
    print(1 / x)
          ~~^~~
ZeroDivisionError: division by zero

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/talley/dev/self/psygnal/x.py", line 11, in <module>
    sig.emit(0)
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1025, in emit
    self._run_emit_loop(args)
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1052, in _run_emit_loop
    raise EmitLoopError(exc=e, signal=self) from e
psygnal.EmitLoopError:

While emitting signal "<SignalInstance 'sig'>", an error occurred in a callback:
  File /Users/talley/dev/self/psygnal/x.py:8, in some_func
  With local variables:
    x = 0

See ZeroDivisionError above for details.

but a slight modification of:

            try:
                with Signal._emitting(self):
                    # allow receiver to query sender with Signal.current_emitter()
                    self._run_emit_loop_inner()
            except Exception as e:
                raise EmitLoopError(exc=e, signal=self).with_traceback(
                    e.__traceback__
                ) from None

would make it look like this:

Traceback (most recent call last):
  File "/Users/talley/dev/self/psygnal/x.py", line 11, in <module>
    sig.emit(0)
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1025, in emit
    self._run_emit_loop(args)
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1052, in _run_emit_loop
    raise EmitLoopError(exc=e, signal=self).with_traceback(
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1045, in _run_emit_loop
    self._run_emit_loop_inner()
  File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1061, in _run_emit_loop_immediate
    caller.cb(args)
  File "/Users/talley/dev/self/psygnal/src/psygnal/_weak_callback.py", line 357, in cb
    self._f(*self._args, *args, **self._kwargs)
  File "/Users/talley/dev/self/psygnal/x.py", line 8, in some_func
    print(1 / x)
          ~~^~~
psygnal.EmitLoopError:

While emitting signal "<SignalInstance 'sig'>", an error occurred in a callback:
  File /Users/talley/dev/self/psygnal/x.py:8, in some_func
  With local variables:
    x = 0

See ZeroDivisionError above for details.

i think that reads a bit easier since it starts with emit(0) and ends with print(1/x) ... however, the details of what goes on in the middle with psygnal is slightly confusing, since the raise EmitLoopError ends up coming in the middle.

@jni, I know that's a lot of text to look at, but if you have a thought/opinion here I'd welcome it (again, the goal is making event callbacks easier to debug)

jni commented 7 months ago

I like your modification @tlambert03! It is indeed much clearer — I always thought the "this is not a bug in psygnal" message sounded kinda cocky. 😂

Side note: why is it called EmitLoopError? I might call it EmitCallbackError? Am I misunderstanding something about the source of the error?

tlambert03 commented 7 months ago

Cocky?! There are plenty of other places that could raise exceptions that would be a bug in psygnal. This particular place though is one where I've seen people (on forums) mistakenly think they had a psygnal issue. Hence the clarification.

But anyway, it's going away now :)

So, which traceback did you prefer? The one using with_traceback or not?

tlambert03 commented 7 months ago

why is it called EmitLoopError? I might call it EmitCallbackError?

i guess it came from "an error that occurs while looping through callbacks during an emit" ... which i term the emit loop. But EmitCallbackError also makes sense, and does indeed point more clearly at the source of the error (instead of the context of the error). I dunno 🤷 ... things like "RuntimeError" sound like "an error that happened in the context of runtime". but i other things like "FileNotFoundError" are cause by the file not being found. hard to pick :)

jni commented 7 months ago

Cocky?! There are plenty of other places that could raise exceptions that would be a bug in psygnal.

😂 haha it's just the certainty of the thing, like, no callback-calling + traceback-catching system I ever devised would make me confident enough to think that it could never be me. 😂 ie I would have chucked a "probably" or "almost certainly" in there 😂

Of course I understand the source of the message. But the new message is better. 😊

So, which traceback did you prefer?

Yes, the one using with_traceback. It seems much easier to find the relevant information.

from "an error that occurs while looping through callbacks during an emit" ... which i term the emit loop

that is where my thoughts went but I think the existence of a for-loop should not really determine the error name...

does indeed point more clearly at the source of the error

I think this is important here. 😊

tlambert03 commented 7 months ago

ok, cleaned up the stack inspection a bit to deal with differences between compiled and uncompiled stacks. This is the final rendering now of the traceback

Traceback (most recent call last):
  File "/Users/talley/dev/self/psygnal/x.py", line 9, in <module>
    sig.emit(0)
  File "src/psygnal/_signal.py", line 1176, in emit
    self._run_emit_loop(args)
  File "src/psygnal/_signal.py", line 1207, in _run_emit_loop
    raise loop_err from cb_err  # emit() call ABOVE || callback error BELOW
  File "src/psygnal/_signal.py", line 1196, in _run_emit_loop
    self._run_emit_loop_inner()
  File "src/psygnal/_signal.py", line 1219, in _run_emit_loop_immediate
    caller.cb(args)
  File "src/psygnal/_weak_callback.py", line 354, in cb
    def cb(self, args: tuple[Any, ...] = ()) -> None:
  File "/Users/talley/dev/self/psygnal/x.py", line 7, in some_func
    print(1 / x)
          ~~^~~
psygnal._exceptions.EmitLoopError: 

While emitting signal <SignalInstance 'sig'>, a ZeroDivisionError occurred in a callback:

  Signal emitted at: /Users/talley/dev/self/psygnal/x.py:9, in <module>
    >  sig.emit(0)

  Callback error at: /Users/talley/dev/self/psygnal/x.py:7, in some_func
    >  print(1 / x)
    Local variables:
       x = 0

See ZeroDivisionError above for original traceback.
tlambert03 commented 7 months ago

also made sure it works for deeply nested signals, which will look like this:

full traceback ```pytb Traceback (most recent call last): File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1196, in _run_emit_loop self._run_emit_loop_inner() File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1221, in _run_emit_loop_immediate caller.cb(args) File "/Users/talley/dev/self/psygnal/src/psygnal/_weak_callback.py", line 357, in cb self._f(*self._args, *args, **self._kwargs) File "/Users/talley/dev/self/psygnal/x.py", line 23, in cb3 1.0 / value ~~~~^~~~~~~ ZeroDivisionError: float division by zero The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/Users/talley/dev/self/psygnal/x.py", line 29, in sig.emit(1) File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1176, in emit self._run_emit_loop(args) File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1204, in _run_emit_loop raise cb_err File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1196, in _run_emit_loop self._run_emit_loop_inner() File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1221, in _run_emit_loop_immediate caller.cb(args) File "/Users/talley/dev/self/psygnal/src/psygnal/_weak_callback.py", line 357, in cb self._f(*self._args, *args, **self._kwargs) File "/Users/talley/dev/self/psygnal/x.py", line 11, in cb1 sig.emit(2) File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1176, in emit self._run_emit_loop(args) File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1204, in _run_emit_loop raise cb_err File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1196, in _run_emit_loop self._run_emit_loop_inner() File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1221, in _run_emit_loop_immediate caller.cb(args) File "/Users/talley/dev/self/psygnal/src/psygnal/_weak_callback.py", line 357, in cb self._f(*self._args, *args, **self._kwargs) File "/Users/talley/dev/self/psygnal/x.py", line 18, in cb2 sig.emit(0) File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1176, in emit self._run_emit_loop(args) File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1209, in _run_emit_loop raise loop_err from cb_err # emit() call ABOVE || callback error BELOW ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1196, in _run_emit_loop self._run_emit_loop_inner() File "/Users/talley/dev/self/psygnal/src/psygnal/_signal.py", line 1221, in _run_emit_loop_immediate caller.cb(args) File "/Users/talley/dev/self/psygnal/src/psygnal/_weak_callback.py", line 357, in cb self._f(*self._args, *args, **self._kwargs) File "/Users/talley/dev/self/psygnal/x.py", line 23, in cb3 1.0 / value ~~~~^~~~~~~ psygnal.EmitLoopError: ```
Traceback (most recent call last):
  ...
  File "/Users/talley/dev/self/psygnal/x.py", line 23, in cb3
    1.0 / value
    ~~~~^~~~~~~
psygnal.EmitLoopError: 

While emitting signal <SignalInstance>, a ZeroDivisionError occurred in a callback
nested 2 levels deep.
(A callback triggered by a signal, emitted by a signal, emitted by a signal):

  Signal emitted at: /Users/talley/dev/self/psygnal/x.py:18, in cb2
    >  sig.emit(0)

  Callback error at: /Users/talley/dev/self/psygnal/x.py:23, in cb3
    >  1.0 / value
    Local variables:
       value = 0

See ZeroDivisionError above for original traceback.
tlambert03 commented 7 months ago

i think the codecov thing is wrong. closing and reopening