jupyter / jupyter_client

Jupyter protocol client APIs
https://jupyter-client.readthedocs.io
BSD 3-Clause "New" or "Revised" License
390 stars 284 forks source link

add KernelManager.exit_status #958

Open jakkdl opened 1 year ago

jakkdl commented 1 year ago

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

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

kevin-bates commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Thanks for looking into this!

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

kevin-bates commented 1 year ago

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 commented 1 year ago
* 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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