Clarify restart to mean restart in-place #118
Conversation
| # Guide-level explanation | ||
|
|
||
| The [protocol](https://jupyter-client.readthedocs.io/en/latest/messaging.html#kernel-shutdown) would describe | ||
| restart as restarting only the kernel process and its subprocess *not* any parent process. |
There was a problem hiding this comment.
I would be a little less specific, and encourage restart to preserve as many resources as make sense, up to but not including the kernel process.
For example, restarting a kernel inside a simple container means the kernel can't be PID 1. Running a kernel via docker exec so it can be restarted is definitely nice, but I don't think it should be a requirement. Both implementations should be considered valid spec-wise, and are a deployment configuration choice, not a protocol choice.
I wouldn't consider any current 'hard restarts' to be protocol violations, rather less optimized implementations, and multiple provider-specific behaviors governed by config can make perfect sense without being dictated by the spec.
There was a problem hiding this comment.
LMK what you think of the changes here and in https://github.com/jupyter/jupyter_client/pull/966/files.
There was a problem hiding this comment.
Before casting my vote, is it possible to get an answer on this comment? I find the raised point A restart should optimally preserve as many resources outside the kernel as possible important, and I don't see it really adressed in the current text of the JEP.
There was a problem hiding this comment.
Thanks for flagging.
I think there is implicit approval from @minrk here—but maybe @minrk can explicitly comment/approve?
@mlucool updated the documentation in the jupyter_client PR with this new wording and @minrk approved that PR after those changes were made (that's what I mean by implicit approval).
Personally, I think this wording is good. As Min mentioned, "I wouldn't consider any current 'hard restarts' to be protocol violations, rather less optimized implementations"—but the spirit of this JEP is to provide a recommendation for how to implement "restart" on the kernel side so that the experience is consistent across kernel implementation (e.g. remote vs local kernels).
How do you feel about the current language here, @echarles? Are you concerned with how this sentence reads, or that it wasn't explicitly accepted by Min after he called it out?
There was a problem hiding this comment.
@Zsailer I also tried to track the timeline of the comments and @minrk approval... and trying to make sense of the current text vs the PR text opened by @mlucool in the jupyter-client. Globally, I would rather have jupyter-client link to this JEP for the details, rather than having two parallel texts subject to interpretation. Maybe I am looking too much at the details... I am OK if someone closes this comment without further details, but would for sure prefer having the clarifications I am missing in the text rather than in the comments.
There was a problem hiding this comment.
@Zsailer Thank your for your time to bring explanation and clarification. The kernel protocol specs living in jupyter-client as "source of thuth" is part of the "debt" we need to work on.
Maybe we should make it explicit in the JEP that we're adding language to the protocol specification in jupyter_client and add the link to the draft PR. What do you think, @echarles?
Agree, so this JEP should just link to @mlucool PR. Once we get enough votes here, we merge both, right?
There was a problem hiding this comment.
Yeah, that's right. @mlucool, does this summary accurately explain the spirit of this JEP?
There was a problem hiding this comment.
Yes. Did you want me to link back now?
There was a problem hiding this comment.
maybe @minrk can explicitly comment/approve?
Yes, as indicated by my explicit approval of both PRs after this discussion, I approve it all as it is now.
There was a problem hiding this comment.
Thx for clarification. I have given a positive vote on this PR.
|
ping @jupyter/software-steering-council for review |
|
I think this and jupyter/jupyter_client#966 are good as they are. I think it's clear now that this is a clarification and suggestion for meeting user expectations, and not a protocol-level change or compatibility issue. |
|
Hey @jupyter/software-steering-council 👋 Since this is a fairly small, straightforward JEP that simply clarifies some expectations around the kernel protocol and discussion has been fairly quite, I propose that we move this into a voting phase by next week. If you have any concerns/comments, please raise them immediately. Thanks all! |
|
Moving this to a voting phase. Pinging the @jupyter/software-steering-council for votes. This vote should end on November 13th, 2023. Voting from @jupyter/software-steering-council
|
|
Thank you @mlucool for all the time you put into writing this JEP and your patience in the review process. Congratulations on the acceptance! 🚀 |
|
Thanks all! |
Resolves #117