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.
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 JEP for sub-shells #91
Add JEP for sub-shells #91
Changes from 1 commit
ac519a0
d389802
abe9275
1b19dde
1f1ad3d
30058d0
5f8bf40
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the user should be able to provide a shell ID in the content, in case they want to reuse it later, instead of getting the shell ID in the reply.
If the provided shell ID already exists, an error would be sent in the reply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general, the entity creating the resource should get to decide the id. Otherwise it turns into a guessing game picking an unused id.
So +1 to the kernel answering back with the subshell id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you envision sub-shells having any properties or inputs? Or are they all by definition identical for a given kernel (at least to start)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only setting I can think of, since the difference between sub-shells and the main shell is that they run concurrently, would be to specify which concurrency "backend" should be used: a thread or a process or asynchronous programming.
But I think it would lead to too much complexity and threading will always be used anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not yet specified exactly where exactly the shell id is passed on the shell messages. I imagine a
shell_id
field in the header (both request to route and response to identify) should suffice. I don't think it should be added tocontent
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the shell ID should be passed in the header of request messages.
Should it be copied in the header of the reply or is it enough for it to be present in the parent header (since it's included in the reply)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to header, especially since many shell message types may use subshells, such as comm messages and autocomplete or inspect requests.
I think it's fine for it to be in the parent_header of the reply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, given that a frontend will likely have to route busy/idle messages, output messages, etc. based on their shell id, does it makes sense to have that shell id in the header of any messages coming from the subshell on the iopub channel, not just shell reply messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should already be in the parent header, which is also included in the messages coming from the subshell on the iopub channel, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, semantics of subshells for kernels that have not implemented them, i.e., using a subshell does not guarantee computations will run concurrently. Really, the only guarantee is that a specific subshell's computations will be run in order (and for a kernel not implementing subshells, this is done by just serializing all shell messages on to the main shell thread).
Also, what happens if you specify a subshell that does not exist on a kernel that supports subshells?
Is there any change to busy/idle message semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the semantics of subshells around interrupt and restart messages, or more generally kernel restarts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jasongrout for the questions, I have opinions but we should discuss these points.
Maybe the
create_subshell_reply
should return an error, and theshell_id
field of any message should be ignored?Again, I think the reply should be an error.
I'm tempted to say that this is a front-end issue. It has all the information about which shell is busy/idle, and it should decide how to translate that information to the user: an OR of all the busy signals, or only select the main shell busy signal.
This almost seems to be orthogonal, as it's about the control channel, which this JEP doesn't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more: what is the lifecycle of subshells around kernel restarts. I think it makes sense for subshells to be terminated, but I also think that should be specified.
If I interrupt the kernel, does it interrupt all shells or just the main shell, or can I selectively interrupt a subshell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that restarting a kernel should terminate subshells, and that interrupting the kernel should interrupt all shells, otherwise it could quickly become a mess. And yes, we should specify it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Starting a new thread for separate conversations)
Currently, in ipykernel, do you know what happens if you send an unrecognized control message? Does it reply with an error, or does it ignore the message?
I think if a shell message is sent with an unknown subshell id, a reasonable response is to ignore the subshell id and schedule the computation on the main shell. That would be backwards compatible with kernels that do not recognize subshell id info, and still guarantees that subshell requests are executed in order.
In other words, I think it is reasonable that giving subshell info does not guarantee concurrency with other messages. It only guarantees that subshell messages will be processed in order, and it is an optimization/implementation detail that messages on one subshell can be processed concurrently with another subshell's messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ipykernel ignores the message, but I don't think the kernel protocol specifies this behavior.
I agree that a kernel that doesn't support subshells should ignore
shell_id
s, and process everything in the main shell, but should a kernel that supports subshells process messages with an unknownshell_id
in the main shell, or reply with an error?Agreed. Also, this JEP doesn't specify the concurrency backend. It will most likely use threads, but we can imagine that a kernel uses e.g.
asyncio
. In this case, concurrency would work only if cells are "collaborative", i.e. theyawait
(a bit like in akernel).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the kernel_info reply carries with it the kernel protocol the kernel speaks, the client will also know what messages the kernel understands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Then I'm wondering if we should even try to be backwards-compatible, since a client knowing that the kernel doesn't support sub-shells should not use them. Maybe we should remove this section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to be backwards compatible. It will be a long hard process to get many shells to support this subshell id (see what @krassowski mentioned in #91 (comment)). It will be much simpler from the client perspective if you can write one codebase that works with both the current protocol and the new protocol with reasonable degradations for the current protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I do wonder if instead of incrementing the kernel protocol number, we should instead have the idea of kernel extensions, e.g., a kernel can advertise which messages it supports, and doesn't have to support other messages in previous kernel protocol versions. For example, I can see a kernel wanting to support subshells before it supports debugging, but there would be no way for a kernel to tell that to a client.
@minrk - what do you think about introducing a field in the kernel info reply for a kernel to advertise which messages it supports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we're starting to increasingly implement additional features in the protocol that are optional for kernels, this makes sense. If we're doing that, is there a more general 'feature' list that can't be represented purely as support for a given message type (e.g. subset of capabilities of a given message type).
I think that should probably be a separate proposal, though.
I think we should probably define a response pattern for kernels to use for unrecognized/unsupported messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually already do it for the debugger (a field indicates whether the kernel supports the debugger). I agree that generalizing it as a list would be a nice improvement. Working on a JEP.