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

add KernelManager.exit_status #958

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jakkdl
Copy link

@jakkdl jakkdl commented Jul 19, 2023

Quick draft implementation of #957 in the way suggested by @kevin-bates

  • I'm not sure what value to return if not self.has_kernel, both 0 and None seem plausible - and could also go with a positive value.
    • or the value can maybe be saved away so it can be accessed even after doing cleanup.

I tested all possible signals, and a bunch of them were ignored - including signal.SIGINT. Not sure if that's intended.

@kevin-bates
Copy link
Member

I'm not sure what value to return if not self.has_kernel, both 0 and None seem plausible - and could also go with a positive value.

A 0 value seems reasonable at this time.

I tested all possible signals, and a bunch of them were ignored - including signal.SIGINT. Not sure if that's intended.

Kernel implementations typically handle SIGINT since that's what interrupt_kernel() uses. So, it would appear as though its ignored. I believe some also ignore SIGTERM (and probably others, depending).

@jakkdl
Copy link
Author

jakkdl commented Jul 20, 2023

@tmaxwell-anthropic any opinions on the detailed behaviour, or in what ways it would be maximally useful for cases such as yours? See in-line comments.

Fixed CI issues*, and testing to see how signals that are defined on windows behave.

* why is mypy not configured to be run in pre-commit? And python_version=3.8 should probably be set in the config. I could open up a separate PR for that.

@tmaxwell-anthropic
Copy link

Thanks for looking into this!

I don't know much about Jupyter internals, but here are some thoughts:

  • What happens if the kernel dies, but then the auto-restarter restarts it? Ideally, the user should be notified that the kernel was restarted, and also why it was restarted (which signal killed it). It looks like the restarter delivers a callback every time the kernel dies or is restarted. Maybe we want to include the exit code in that callback?

  • I don't know how the kernel restarter interacts with intentional shutdown. Ideally the user can tell the difference between whether Jupyter intentionally shut down the kernel, versus the kernel unexpectedly calling something like os._exit(0).

@kevin-bates
Copy link
Member

The auto-restarter is stopped for any flavor of intentional "shutdown" (either directly or on behalf of intentional restarts). It lives only to detect unexpected terminations.

@jakkdl
Copy link
Author

jakkdl commented Jul 31, 2023

* What happens if the kernel dies, but then the auto-restarter restarts it? Ideally, the user should be notified that the kernel was restarted, and also why it was restarted (which signal killed it). It looks like the restarter delivers a callback every time the kernel dies or is restarted. Maybe we want to include the exit code in that callback?

Adding a parameter with the exit code to the callback would either break all existing code, or require inspecting the callable to see if it can take a parameter with the signature or not. The latter is definitely doable, but not sure if it's the best way go to about it.

@tmaxwell-anthropic
Copy link

Another option might be to add a second set of callbacks, with different names, which are equivalent except that they take an extra parameter. So the component that's registering the callback could indicate whether it wants a parameter based on which callback name it registers for.

@jakkdl
Copy link
Author

jakkdl commented Aug 1, 2023

Another option might be to add a second set of callbacks, with different names, which are equivalent except that they take an extra parameter. So the component that's registering the callback could indicate whether it wants a parameter based on which callback name it registers for.

Ah that's pretty nice. Or probably even cleaner, add a parameter accepts_exit_code: bool = False to add_restart_callback and save it alongside it.

@jakkdl
Copy link
Author

jakkdl commented Aug 17, 2023

made a somewhat messy (esp when you look at the typing) implementation for restarter callback taking the exit status, and wrote (copy-paste+modified) a test for it. A more creative test would kill the process in several different ways, but that should be covered by TestKernelManagerExitStatus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants