Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

tlambert03
Copy link
Member

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.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

Copy link

codspeed-hq bot commented Mar 15, 2024

CodSpeed Performance Report

Merging #302 will not alter performance

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

Summary

✅ 66 untouched benchmarks

@tlambert03
Copy link
Member Author

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
Copy link

jni commented Mar 15, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link

jni commented Mar 15, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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

full traceback
  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 <module>
    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
Copy link
Member Author

i think the codecov thing is wrong. closing and reopening

@tlambert03 tlambert03 closed this Mar 28, 2024
@tlambert03 tlambert03 reopened this Mar 28, 2024
@tlambert03 tlambert03 merged commit 1c6f140 into pyapp-kit:main Mar 28, 2024
54 checks passed
@tlambert03 tlambert03 deleted the exception-stack branch March 28, 2024 21:42
@tlambert03 tlambert03 added the enhancement New feature or request label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants