Python: don't catch KeyboardInterrupt and SystemExit
#4333
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are little reasons to emit a warning and keep-on going when a KeyboardInterrupt exception is received (i.e. was sent by the user). I propose to let this one be passed to the client even in non strict mode
Dvad
changed the title
KeyboardInterrupt to the user when emitted in log function.KeyboardInterrupt to the user when emitted in log function.
emilk
approved these changes
Nov 26, 2023
emilk
previously requested changes
Nov 26, 2023
| @@ -205,7 +205,7 @@ def __exit__( | |||
| exc_tb: TracebackType | None, | |||
| ) -> bool: | |||
| try: | |||
| if exc_type is not None and not strict_mode(): | |||
| if exc_type is not None and exc_type is not KeyboardInterrupt and not strict_mode(): | |||
There was a problem hiding this comment.
I believe this is the better fix - this will ignore both KeyboardInterrupt and SystemExit
Suggested change
| if exc_type is not None and exc_type is not KeyboardInterrupt and not strict_mode(): | |
| if exc_type is Exception and not strict_mode(): |
There was a problem hiding this comment.
If that's the intention, perhaps worth adding a comment to the effect as well?
There was a problem hiding this comment.
@emilk well spotted (I had forgotten about that Exception vs. BaseException thing). I fixed the code with the correct issubclass check + a comment.
emilk
changed the title
KeyboardInterrupt to the user when emitted in log function.KeyboardInterrupt and SystemExit
jleibs
approved these changes
Nov 27, 2023
abey79
dismissed
emilk’s stale review
Changes were made, but needed to unblock merge.
emilk
approved these changes
Nov 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Currently, the python sdk will catch any exception raised inside a rerun log function and surface it as a printed warning rather than an exception to the client. This makes sense to avoid crashing the app for a mistake in the argument of the log function. This can be disabled with the "strict mode" through an environment variables.
However, there are little reasons to emit a warning and keep-on going when a
KeyboardInterruptexception is received. The official doc states even that this kind of exception are better not being caught at all:I propose to have a special case for this type of exception, and throwing them back to the client even in non strict mode.
I tested, that Ctrl-C now interrupts some test program instantly instead of being unreliable before (because sometime catches inside rerun log).
Checklist