Updates to the kernel messaging spec #388
Conversation
…lient or the kernel.
These messages were actually required, but you had to read the status message documentation to realize they were required. This makes the requirement more explicit.
In conversation with @minrk, we decided shutdown messages should always be treated with a higher priority, and it was confusing to have them sent on either channel.
1. Make the ‘starting’ status optional, since receiving it is unreliable anyway due to race conditions with connecting to the kernel. 2. Clarify that the kernel may send other status states that could be ignored 3. Take out the notebook-specific states, since that should be in notebook documentation, not in kernel messaging docs.
docs/messaging.rst
Outdated
| all clients connected to a kernel and should be constant over the lifetime of | ||
| the client. A kernel session value, in message headers from a kernel, should be | ||
| generated on kernel startup or restart and should be constant for the lifetime | ||
| of the kernel. |
There was a problem hiding this comment.
Thanks for clarifying this one in the docs!
| @@ -959,6 +967,12 @@ Message type: ``shutdown_reply``:: | |||
| socket, they simply send a forceful process termination signal, since a dead | |||
| process is unlikely to respond in any useful way to messages. | |||
|
|
|||
| .. versionchanged:: 5.4 | |||
|
|
|||
| Sending a ``shutdown_request`` message on the ``shell`` channel is deprecated. | |||
There was a problem hiding this comment.
What's the strategy for deprecating the shutdown_request?
We also deprecated payloads, yet we still use them and have no current replacement.
There was a problem hiding this comment.
Are you basically asking what deprecation means in this context? Good question. @minrk, thoughts? Does this mean that if a kernel implements protocol 5.4, it doesn't need to listen to shutdown_request messages on the shell channel?
There was a problem hiding this comment.
Actually, that doesn't make much sense to me. How about this: we're just warning people now, and a kernel can officially ignore shutdown requests on the shell channel in spec 6.0 or later.
There was a problem hiding this comment.
Works for me!
Is most of the intent behind this that the shutdown API from the server is used? Was shutdown request put in place to help ensure that background resources got cleaned up appropriately (like with a cluster, deleting all the executors / workers?)
There was a problem hiding this comment.
IIRC, the kernel manager in the notebook server first sends a shutdown request to the kernel to let it clean up resources and exit by itself. If it doesn't receive a reply in a certain amount of time, then it forcefully shuts down the kernel with a signal.
This protocol definition is at the kernel level, not the kernel manager level. At this kernel message level, if I remember what @minrk said, sending the shutdown_request on either the shell or control channel is an artifact of history, and probably shouldn't have been done that way.
There was a problem hiding this comment.
The notebook server uses shutdown_request on the control channel as a first, polite "please shutdown" before sending signals. This is deprecating the ~unused functionality of sending these messages on the shell channel. Does nteract send shutdown_request on either of these channels?
@jasongrout I notices that KernelClient.shutdown is sending it on the shell channel. This is a rarely, if ever used method, that I think would only be used by frontends that don't own the kernel (e.g. qtconsole --existing), so that should be updated.
There was a problem hiding this comment.
I am +1 on disallowing the shutdown and interupt on shell.
I don't know of any kernel that do it anyways since our clients use the control channel for that.
Making this explicit is really important IMO.
|
The part I deleted about the notebook kernel status messages I filled out into more detailed docs for interacting with a kernel through the notebook server: jupyter/notebook#3765 |
|
@SylvainCorlay and @JohanMabille - can you comment on why sending the shutdown message on the shell channel is a problem for you? |
|
@jasongrout the client may want to restart the kernel while a request is being handled (and possibly stalled) on the shell channel. In that case, we want to be able to interrupt the execution of that request. Having the guarantee that the shutdown message is sent on the control channel gives more flexibility when implementing the protocol and is more consistent with the general description of the control channel. |
|
@JohanMabille - so we certainly agree that it usually makes more sense to send the shutdown request on the control channel, exactly for the message priority issues you mentioned.
This is what we were looking for - How is it more flexible when there is the guarantee that the shutdown only comes on the control channel? |
|
The flexibility (maybe simplicity is more appropriate) comes from the possibility to have one thread per channel, thus you can shutdown a kernel even if the shell thread is processing a request. With the current specification, you have to synchronize a thread that processes the requests with a thread that polls both control and shell sockets. |
|
@minrk - please review the changes I just pushed. Opening a control channel for the shutdown message, and percolating it through the code, is actually much more invasive than I thought originally. |
docs/messaging.rst
Outdated
| A kernel may send optional status messages with execution states other than | ||
| `busy` or `idle`. For example, a kernel may send a status message with a | ||
| `starting` execution state exactly once at process startup. | ||
|
|
There was a problem hiding this comment.
@minrk, do we know of other kernels publishing other status states? We made the 'starting' status optional because there might be a race condition preventing the client from ever seeing a starting message. However, it is nice if we have a universe of only three status states, rather than opening it up to any status the kernel might want to send.
As for the kernel status states that the notebook spoofs, we already agreed that those should come on a different channel, a notebook channel, so I think those status states shouldn't count here as optional states sent from a kernel.
There was a problem hiding this comment.
To be clear, I think I'm +1 on reverting this "optional status states" change, and just going back to mandating a 'starting' status, and having a defined universe of 'starting', 'busy', and 'idle' states sent from the kernel.
There was a problem hiding this comment.
I don't have an opinion on this.
In particular, the signatures of some functions changed, so this might now be considered a backwards-incompatible change, depending on what we view as the public-facing API of the package. |
| The session id in a message header can be used to identify the sending entity. | ||
| For example, if a client disconnects and reconnects to a kernel, and messages | ||
| from the kernel have a different kernel session id than prior to the disconnect, | ||
| the client should assume that the kernel was restarted. |
| @@ -934,8 +954,7 @@ multiple cases: | |||
|
|
|||
| The client sends a shutdown request to the kernel, and once it receives the | |||
| reply message (which is otherwise empty), it can assume that the kernel has | |||
| completed shutdown safely. The request can be sent on either the `control` or | |||
| `shell` channels. | |||
| completed shutdown safely. The request is sent on the `control` channel. | |||
There was a problem hiding this comment.
👍 this is really important this is specified here. +1000
| @@ -959,6 +967,12 @@ Message type: ``shutdown_reply``:: | |||
| socket, they simply send a forceful process termination signal, since a dead | |||
| process is unlikely to respond in any useful way to messages. | |||
|
|
|||
| .. versionchanged:: 5.4 | |||
|
|
|||
| Sending a ``shutdown_request`` message on the ``shell`` channel is deprecated. | |||
There was a problem hiding this comment.
I am +1 on disallowing the shutdown and interupt on shell.
I don't know of any kernel that do it anyways since our clients use the control channel for that.
Making this explicit is really important IMO.
docs/messaging.rst
Outdated
| A kernel may send optional status messages with execution states other than | ||
| `busy` or `idle`. For example, a kernel may send a status message with a | ||
| `starting` execution state exactly once at process startup. | ||
|
|
There was a problem hiding this comment.
I don't have an opinion on this.
|
@jasongrout is this still a WIP PR? |
The WIP part is that we wanted to revert the part about optional status messages. |
…message states. We still remove the part about spoofed status messages since it really is not part of the kernel message spec.
jasongrout
changed the title
|
I just pushed a commit reverting the status message changes. I think this is now ready to go and has addressed the concerns raised in the comment process over the last year. |
|
Awesome. I am 👍 for merging now. Leaving this open for a bit more to give others the occasion to comment! cc @minrk |
|
Cc @JohanMabille. This is in! |
These came out of conversations with @minrk.
CC also @SylvainCorlay, who I think has been advocating for the deprecation of sending a shutdown message on the shell channel.
Marking as work-in-progress since I plan to work on this more in the near future. But I also think these changes are pretty self-contained, so I wouldn't regret us merging these now and submitting another PR later either.