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 JEP for sub-shells #91

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Conversation

davidbrochart
Copy link
Member

@davidbrochart davidbrochart commented Dec 15, 2022

This PR introduces kernel sub-shells to allow for concurrent code execution.
It is a JEP because it changes the kernel messaging protocol - these changes consist of (optional) additions.

Voting from @jupyter/software-steering-council

@davidbrochart
Copy link
Member Author

@minrk it would be great if you could be the shepherd for this JEP.

'status': 'ok',

# The ID of the sub-shell.
'shell_id': str
Copy link
Member

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)?

Copy link
Member Author

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.

# Points of discussion

The question of sub-shell ownership and life cycle is open, in particular:
- Is a sub-shell responsible for deleting itself, or can a shell delete other sub-shells?
Copy link
Member

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 to content.

Copy link
Member Author

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)?

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for it to be in the parent_header of the reply.

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?

Copy link
Member Author

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?

@davidbrochart
Copy link
Member Author

I started an implementation of sub-shells in ipykernel, that can be useful to validate the kernel protocol changes required by this JEP.

@davidbrochart
Copy link
Member Author

Although tests fail in ipython/ipykernel#1062, it is functional (you can try it e.g. with JupyterLab) and the sub-shell test shows that sub-shells run in parallel.


## Create sub-shell

Message type: `create_subshell_request`: no content.
Copy link
Member Author

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.

Copy link
Member

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.

@davidbrochart
Copy link
Member Author

I opened a PR in JupyterLab that uses the ipykernel PR to implement "sub-consoles". Together, these PRs can serve as a proof-of-concept for this JEP.

- Is a sub-shell responsible for deleting itself, or can a shell delete other sub-shells?
- Can a sub-shell create other sub-shells?
- Does sub-shells have the same rights as the main shell? For instance, should they be allowed to
shut down or restart the kernel?
Copy link
Member

@jasongrout jasongrout Jan 7, 2023

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?

Copy link
Member

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?

Copy link
Member Author

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.

Also, semantics of subshells for kernels that have not implemented them

Maybe the create_subshell_reply should return an error, and the shell_id field of any message should be ignored?

Also, what happens if you specify a subshell that does not exist on a kernel that supports subshells?

Again, I think the reply should be an error.

Is there any change to busy/idle message semantics?

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.

What are the semantics of subshells around interrupt and restart messages, or more generally kernel restarts?

This almost seems to be orthogonal, as it's about the control channel, which this JEP doesn't change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This almost seems to be orthogonal, as it's about the control channel, which this JEP doesn't change.

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?

Copy link
Member Author

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 👍

@jasongrout
Copy link
Member

A huge +1 to exploring this idea - thanks @davidbrochart!

Recently we've been having a number of cases where we'd like computations to run concurrently with normal execute requests (like autocomplete, some widget comm messages, etc.). Having some sort of way to run these would make a huge difference.

- Is a sub-shell responsible for deleting itself, or can a shell delete other sub-shells?
- Can a sub-shell create other sub-shells?
- Does sub-shells have the same rights as the main shell? For instance, should they be allowed to
shut down or restart the kernel?
Copy link
Member

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)

Also, semantics of subshells for kernels that have not implemented them

Maybe the create_subshell_reply should return an error, and the shell_id field of any message should be ignored?

Also, what happens if you specify a subshell that does not exist on a kernel that supports subshells?

Again, I think the reply should be an error.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

ipykernel ignores the message, but I don't think the kernel protocol specifies this behavior.

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.

I agree that a kernel that doesn't support subshells should ignore shell_ids, and process everything in the main shell, but should a kernel that supports subshells process messages with an unknown shell_id in the main shell, or reply with an error?

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.

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. they await (a bit like in akernel).

Copy link
Member

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.

Since the kernel_info reply carries with it the kernel protocol the kernel speaks, the client will also know what messages the kernel understands.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about introducing a field in the kernel info reply for a kernel to advertise which messages it supports?

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 don't think the kernel protocol specifies this behavior.

I think we should probably define a response pattern for kernels to use for unrecognized/unsupported messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should probably be a separate proposal, though.

+1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about introducing a field in the kernel info reply for a kernel to advertise which messages it supports?

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.

@krassowski
Copy link
Member

This might be a first step to enable a highly-requested feature: built-in variable explorer (e.g. jupyterlab/jupyterlab#443) which would not depend on the debugger being active (or supported by the kernel for that matter, thinking about R here), do I see this right?

I wonder how to avoid the problem that we had/have with the debugger which was the slow adoption across kernels. Are there any plans to bootstrap this by providing pull requests to a few kernel with largest user base (e.g. Julia/R/pyodide kernels) - in addition to IPython/xeus stack - so that there are a few implementation examples available for other kernel maintainers to derive from?

@davidbrochart
Copy link
Member Author

This might be a first step to enable a highly-requested feature: built-in variable explorer (e.g. jupyterlab/jupyterlab#443) which would not depend on the debugger being active (or supported by the kernel for that matter, thinking about R here), do I see this right?

Yes, it could allow a variable explorer to be responsive while the kernel is busy.

I wonder how to avoid the problem that we had/have with the debugger which was the slow adoption across kernels. Are there any plans to bootstrap this by providing pull requests to a few kernel with largest user base (e.g. Julia/R/pyodide kernels) - in addition to IPython/xeus stack - so that there are a few implementation examples available for other kernel maintainers to derive from?

I have not looked at kernels for other languages, but only in ipykernel this requires some architectural changes: reading the shell channel messages in a separate thread (usually this is done in the main thread), and processing the sub-shell execute requests in separate threads. For ipykernel, this also means getting rid of Tornado, and using asyncio, so quite a lot of work. So it depends on the code base of each kernel, I cannot say in advance if it would be easy or not. For pyodide, I'm afraid it won't be possible since threads are not supported in WASM, AFAIK.

@davidbrochart
Copy link
Member Author

davidbrochart commented Jan 9, 2023

I made changes according to the comments in d389802. I removed the points of discussion because I think they don't really make sense: the operations I mentioned (create sub-shell, delete sub-shell, kernel shut-down, kernel restart) are on the control channel, which is orthogonal to sub-shells. It is not a sub-shell which deletes/creates another sub-shell, or restarts/shuts-down a kernel, it is the client/user.

A [kernel restart](https://jupyter-client.readthedocs.io/en/stable/messaging.html#kernel-shutdown)
should delete all sub-shells. A
[kernel interrupt](https://jupyter-client.readthedocs.io/en/stable/messaging.html#kernel-interrupt)
should interrupt the main shell and all sub-shells.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that perhaps an interrupt request could give a subshell id to interrupt only that subshell. However, if we want to be backwards compatible, we have to interrupt all shells: if all subshell requests are processed in the main shell, then interrupting the kernel will currently interrupt all shells.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Maybe we could also say that a kernel should do its best at interrupting only the requested sub-shell, but that it may interrupts all shells?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Maybe we could also say that a kernel should do its best at interrupting only the requested sub-shell, but that it may interrupts all shells?

That sounds too unpredictable to me. I think if we want subshell-specific interrupt, we need another message so we can be backwards compatible and predictable.

- deleting a sub-shell,
- listing existing sub-shells.

A sub-shell should be identified with a shell ID, either provided by the client in the sub-shell

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say the kernel always generates the shell id and we don't support the client providing an id. Once you have clients providing ids, then it's always a guessing game if there is contention between clients, or you have clients generate UUIDs, at which point you might as well have the kernel generate a truly unique id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that if in the future we allow per-cell sub-shells (through e.g. cell metadata), it could open up possibilities such that a cell creates a sub-shell, and other cells run in this sub-shell, so they would need the shell ID. We could build complex asynchronous systems like that.
akernel can do something similar but programmatically: __task__() returns a handle to the previous cell task, so the next cell can do whatever it wants with it (await it, etc.).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the client specifies a subshell id, it will need to wait until it is confirmed in the reply to be sure it has reserved that name. In that case, why not just get the subshell id from the reply message, and be guaranteed it didn't fail because of a name conflict? What does having the client give the subshell id do for us?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that it allowed us to reuse it later, at least in the case of a self-contained notebook where we know there is no shell ID conflict.

Copy link
Member

@jasongrout jasongrout Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A notebook might be opened with two copies, in which case each copy would want to start up a subshell with the same name? For example, either a notebook in real-time collaboration, or a notebook opened twice side by side in JLab?

Or perhaps if you try to create a subshell with an existing id, it just acknowledges that the subshell is already created, with no error? Multiple clients might send computations to the same subshell?

What if we treat it like we do kernel sessions right now, with a user-supplied name as a key? In other words, a client subshell creation request optionally gives a name (not an id). If a subshell with that name already exists, its id is returned. If it doesn't exist, a new subshell with that name is created and returned. And if a name is not given by the client, an unnamed subshell is created and returned. Thoughts? This gives you the ability to share subshells between clients addressable with some client-supplied string, but gives me always unique ids created by the resource manager.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps if you try to create a subshell with an existing id, it just acknowledges that the subshell is already created, with no error?

I like that. It seems that there is no distinction between a sub-shell name and a sub-shell ID in this case.

What if we treat it like we do kernel sessions right now, with a user-supplied name as a key?

In that case there seems to be an unnecessary mapping between sub-shell name and sub-shell ID, or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a technical difference: since the client name and shell id are different namespaces, the shell id (generated by the kernel) does not have to check for conflicts with the names already given, the client can request a shell that is guaranteed that no one else will ever request (i.e., a shell specific to itself, guaranteed to not collide with any other requested name).

For example, suppose the shell ids are generated by starting at 0 and incrementing for each new subshell. If the client asks for shell name 5, and the client name and shell id namespaces are conflated, the client won't know if it's getting some random subshell someone else created (i.e., shell id 5), or if it's getting a shell with specific meaning "5" (i.e., client name 5). Likewise, any time a new shell id is generated, the kernel would have to check to see if someone else had already claimed that number.

I think it's a much better design to keep the client-specific names in a separate namespace from the unique shell ids. With this design, any client asking for a shell named "autocomplete" gets the same autocomplete shell shared with every other client requesting the same subshell name. However, if you want to get your own subshell separate from any other client, you just request a subshell without a name.

@JohanMabille JohanMabille mentioned this pull request Jan 16, 2023
33 tasks
@SylvainCorlay
Copy link
Member

Thanks David for starting the discussion on this. I wanted to bring up the alternative design, which is the one I initially proposed for this.

In the proposal described here

There is still only one shell socket, and you address the subshell via a shell Id.

In my alternative design

Subshells open their own shell socket, and appear as a separate kernel in the UI on which you can connect any client.

I strongly prefer this design because:

  • it is only additive (addition of a control message for creating a subshell).
  • it does not require clients to change drastically (like to support shell ids on execution requests) or to specify a subshell id upon connexion.

@kevin-bates
Copy link
Member

hi @SylvainCorlay.

Subshells open their own shell socket, and appear as a separate kernel in the UI on which you can connect any client.

Wouldn't the subshell socket need to be communicated back to the (potentially remote) launching server and does kernel lifecycle management get affected since there are now N additional ports to deal with?

How are subshells terminated? Is it simply a matter of (and only in the case this virtual kernel is a subshell) closing that socket?

I think applications will still need to apply an id (or name) to each subshell (for their own management) and feel an explicit approach (where that id/name is known by the kernel) would be easier to manage/troubleshoot, and less confusing to the end-user.

@jasongrout
Copy link
Member

jasongrout commented Jan 20, 2023

Subshells open their own shell socket, and appear as a separate kernel in the UI on which you can connect any client.

Thinking through how this would work in practice:

Currently we have control, iopub, input, heartbeat, and shell zmq sockets. You're proposing that we now have control, iopub, input, heartbeat, main shell (shell0?), shell1, shell2, ..., shellN sockets that the kernel manager must connect to? I imagine we then create a subshell with a rest api request to the kernel manager (similar to creating a new kernel connection), so the kernel manager knows to connect to the new shell zmq channel.

The current Jupyter web client multiplexes the kernel messages onto a single websocket. How would that multiplexing work in your scenario? There is a new websocket for each shell channel, and control, iopub, input messages are routed to the shared zmq sockets, but the shell messages are routed to the shellN channel for that particular websocket connection?

@SylvainCorlay
Copy link
Member

Thinking through how this would work in practice:

Currently we have control, iopub, input, heartbeat, and shell zmq sockets. You're proposing that we now have control, iopub, input, heartbeat, main shell (shell0?), shell1, shell2, ..., shellN sockets that the kernel manager must connect to? I imagine we then create a subshell with a rest api request to the kernel manager (similar to creating a new kernel connection), so the kernel manager knows to connect to the new shell zmq channel.

The current Jupyter web client multiplexes the kernel messages onto a single websocket. How would that multiplexing work in your scenario? There is a new websocket for each shell channel, and control, iopub, input messages are routed to the shared zmq sockets, but the shell messages are routed to the shellN channel for that particular websocket connection?

The new subshell would appear as a separate running kernel, and frontends would connect to it in the same way as they connect to regular kernels. The fact that a "kernel" that happens to really be a subshell of another one would be mostly transparent to the frontend.

@SylvainCorlay
Copy link
Member

Wouldn't the subshell socket need to be communicated back to the (potentially remote) launching server and does kernel lifecycle management get affected since there are now N additional ports to deal with?

Indeed, the subshell socket would be in the response to the control-channel request to create a subshell. Subshells would also be visible as regular running kernels in the kernels API.

I think applications will still need to apply an id (or name) to each subshell (for their own management) and feel an explicit approach (where that id/name is known by the kernel) would be easier to manage/troubleshoot, and less confusing to the end-user.

I don't think we would need a new ID for subshell in this new proposal (unlike the one presented here), because they would just have a separate kernel ID.

How are subshells terminated? Is it simply a matter of (and only in the case this virtual kernel is a subshell) closing that socket?

This is tan excellent idea. I think that there are two options:

  • the first (bad IMO) option would be to use a regular shutdown request on a (sub)control, or through a shared control channel with the main shell. I am not in favor of this.
  • the second option would be to have a special "control" message to kill a given subshell (referred to by its kernel id)

This second option is somewhat tied to something that we want to propose independently of subshells for kernels whose lifetime are tied to another process (like a desktop application having an embedded kernel), or a main shell. These kernels would be signalled to the frontend, so that it does not include a shutdown UI.

@kevin-bates
Copy link
Member

ok. I think we need to understand and define how similar and different these kinds of "kernels" behave relative to standard kernels and feel they might deserve a different name so we can keep our sanity. It sounds like only the "parent"(?) kernel would be managed by a KernelManager (and KernelProvisioner) and the "others" are dependents of that kernel and essentially managed via messaging - is that a correct statement?

Are these "dependent kernels" independently interruptable and restartable?

Should this be a separate JEP since exposing these as separate kernels will have a significant impact (IMHO)?

@SylvainCorlay
Copy link
Member

It sounds like only the "parent"(?) kernel would be managed by a KernelManager (and KernelProvisioner) and the "others" are dependents of that kernel and essentially managed via messaging - is that a correct statement?

I think that whether it is managed by KernelManager and KernelProvisioner is an implementation detail of jupyter-server.

But yes, subshells are "kernels" that are created by sending a control message to an existing kernel, nd their lifetime is controlled that way - but they have their own connection file and so on.

@kevin-bates
Copy link
Member

I think that whether it is managed by KernelManager and KernelProvisioner is an implementation detail of jupyter-server.

This would be jupyter_client and believe these are details that need to be understood within the JEP. I'm sure this can be worked out but want to make sure we're all on the same page. After all, the devil is in the details. 😄

Is the original proposal that @davidbrochart submitted being abandoned for this alternative approach, or should a separate JEP be submitted? I think these two approaches vary enough to warrant separate discussions and don't want to continue on David's JEP unless this is the plan moving forward.

@jasongrout-db
Copy link

I just noticed there are some assumptions in python around the main thread and signals:

Python signal handlers are always executed in the main Python thread of the main interpreter, even if the signal was received in another thread. This means that signals can’t be used as a means of inter-thread communication. You can use the synchronization primitives from the threading module instead.

Besides, only the main thread of the main interpreter is allowed to set a new signal handler.

I was thinking that the main thread would be routing shell channel messages to subshell threads, one of which would be started by default, but it sounds like instead the main thread should be executing user code, and a separate thread should be routing shell channel messages, so that user code that sets signal handlers still works.

I think this also has implications about interrupting subshells, which currently uses sigint to trigger a KeyboardInterrupt exception.

@jasongrout-db
Copy link

Also, I mentioned recently in our kernels dev meeting that we've been dealing with some issues in the ipython post_execute hooks executing on the control thread. I think we'll need to clarify how those execution hooks (pre_execute, post_execute, pre/post_run_cell, etc.) behave for subshells. I bet there are a lot of ipython execute hook callbacks out there that currently assume single-threaded execution (like the matplotlib display hooks I mentioned in last week's meeting).

@ianthomas23
Copy link
Contributor

I was thinking that the main thread would be routing shell channel messages to subshell threads, one of which would be started by default, but it sounds like instead the main thread should be executing user code, and a separate thread should be routing shell channel messages, so that user code that sets signal handlers still works.

I agree. I made the same original assumption in the demo code that the main thread would handle shell messages and each subshell (parent or child) would run in a new thread, as that was the easiest way to implement the demo. But as you say the parent subshell needs to run in the main thread (for signal handling, GUI event loop support, etc, etc) so the message handler and each child subshell will be in new threads.

@ianthomas23
Copy link
Contributor

Also, I mentioned recently in our kernels dev meeting that we've been dealing with some issues in the ipython post_execute hooks executing on the control thread. I think we'll need to clarify how those execution hooks (pre_execute, post_execute, pre/post_run_cell, etc.) behave for subshells. I bet there are a lot of ipython execute hook callbacks out there that currently assume single-threaded execution (like the matplotlib display hooks I mentioned in last week's meeting).

For the favoured kernel subshells implementation this is an example of how the subshell_id will necessarily appear in messages and hook callbacks, and clients will have to choose if and/or how to use that information. In the same way that an execute_reply_message will contain a subshell_id, so will the info passed to a post_run_cell hook. If a client needs different behaviour based on the subshell triggering the hook, it will have to to do itself. If I understand post_run_cell hooks there is no such information passed so we cannot add a subshell_id, which may be problematic.

@jasongrout
Copy link
Member

If I understand post_run_cell hooks there is no such information passed so we cannot add a subshell_id, which may be problematic.

I think you're right - I think we'd need a different hook to run on non-main subshell executions. We shouldn't change the single-threaded semantics of the existing hooks.

@jasongrout-db
Copy link

@ianthomas23 - are the instructions above at #91 (comment) still the most current instructions for trying out subshells?

CC @ryan-wan

@ianthomas23
Copy link
Contributor

@ianthomas23 - are the instructions above at #91 (comment) still the most current instructions for trying out subshells?

Yes. I have not rebased any of the branches since then.

@ianthomas23
Copy link
Contributor

There is a reference ipykernel implementation of this JEP at ipython/ipykernel#1249. See there for details and how to try it out in binder.

@gabalafou
Copy link
Contributor

Hi, this came up in the SSC call today, and we were curious about the current status of this and if you are waiting on anything from the SSC at this point in time

@ianthomas23
Copy link
Contributor

Hi, this came up in the SSC call today, and we were curious about the current status of this and if you are waiting on anything from the SSC at this point in time

Thanks for the reminder, I don't need anything from the SSC just yet. There are a couple of tasks that need to be performed on the ipykernel reference implementation before then: (1) I need to check details of some of the test failures, and (2) @jasongrout would like to review it. Hopefully they will be completed in the next month and then I'll be asking the SSC for a vote and looking to merge the ipykernel PR in a coordinated way.

@SylvainCorlay
Copy link
Member

This JEP is now open for voting.

@SylvainCorlay
Copy link
Member

Hey @vidartf @rpwagner @gabalafou do you plan on reviewing this, or should we close the vote?

@gabalafou
Copy link
Contributor

Sorry for the delay, I will vote by end of day tomorrow!

@JohanMabille
Copy link
Member

Let's extend the voting period to next Monday, since people may have been in holidays during the past few weeks.

@SylvainCorlay
Copy link
Member

This JEP has been approved! 🎉

  • Yes 7
  • No 0
  • Abstain 1
  • Absence of vote 2

@SylvainCorlay SylvainCorlay merged commit 88ad698 into jupyter:master Sep 9, 2024
1 check passed
@davidbrochart davidbrochart deleted the sub-shells branch September 9, 2024 18:27
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.