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

Support subshells #955

Closed
wants to merge 9 commits into from
Closed

Conversation

davidbrochart
Copy link
Collaborator

This PR aims at implementing potential solutions to support "subshells" (see jupyter/jupyter_client#806).

In this first commit, the main shell runs in its own thread (which is not the main thread). A subshell can be created through a subshell_request received on the shell channel. This creates a new thread for this subshell, identified by a subshell ID. A request targeting this subshell must specify its subshell_id in the metadata (if not specified, then the request is targeting the main shell).
Shell messages are received in the main thread and handled in the thread corresponding to the shell ID (the ID of the main shell is main, the ID of a subshell is a UUID). Thus, it is always possible to receive shell messages, even if a shell request is being handled.
This approach modifies the behavior of ipykernel in the following ways:

  • if cells are cooperative (i.e. they have a top-level await), then they can be executed concurrently, similarly to what akernel does.
  • because the main shell is not executed in the main thread, it can potentially have consequences on code that expects to run in the main thread.
  • the handling of side-effects (e.g. printing to stdout or stderr) currently makes the assumption that shell requests are received and handled one at a time, which is not the case anymore: the next shell request can be received while the previous one is being handled.

@davidbrochart davidbrochart marked this pull request as draft June 14, 2022 07:07
@@ -396,28 +403,33 @@ async def dispatch_shell(self, msg):
self.log.warning("Unknown message type: %r", msg_type)
else:
self.log.debug("%s: %s", msg_type, msg)
asyncio.run_coroutine_threadsafe(self.call_handler(shell_id, handler, idents, msg), self.shell_threads[shell_id].io_loop.asyncio_loop)
Copy link
Contributor

@JohanMabille JohanMabille Jun 14, 2022

Choose a reason for hiding this comment

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

This cannot work because you will send replies on the same socket from two different threads, and ZMQ sockets are NOT thread safe (unless PyZQM adds some synchronization mechanism in the bindings, I need to check). Potentially same issue if you authorize stdin in the subshell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but this can be solved by adding locks when sending messages.

Copy link
Contributor

@JohanMabille JohanMabille Jun 14, 2022

Choose a reason for hiding this comment

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

The whole point of ZeroMQ is to avoid to share sockets (or anything actually) among threads, do not use multithreading primitives to synchronize threads, they should be synchronized via interprocess sockets. Even with a lock, you have no guarantee that the socket will handle both threads correclty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If that is true (I could not find any documentation confirming what you said), then it means that we should use a new ZMQ socket per subshell.

Copy link
Contributor

@JohanMabille JohanMabille Jun 14, 2022

Choose a reason for hiding this comment

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

Here is a link to the specific section of ZeroMQ guide for more detail about my last statement.

EDIT: sorry I missed you answer above, thus my partial answer here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, that is quite clear indeed 😄
"To make utterly perfect MT programs (and I mean that literally), we don’t need mutexes, locks, or any other form of inter-thread communication except messages sent across ZeroMQ sockets."
So, do we agree that we should let down the approach consisting of one ZMQ channel for all subshells, and instead have one ZMQ channel per subshell?

Copy link
Contributor

@JohanMabille JohanMabille Jun 14, 2022

Choose a reason for hiding this comment

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

Indeed I find the second approach cleaner since we delegate the routing to ZeroMQ (although it requires additional changes in the server). However, for the sake of completeness (and if you want to still experiment with the first approach), the way to implement it would be:

  • one thread polling the shell sockets and additional PAIR sockets for communication between the poller and the execution threads.
  • one thread for the main shell and one per subshell.

When a message arrives on the shell, the polling thread parses it and sends it to the corresponding thread via the pair socket. The executing thread should push the answer on that pair socket instead of sending it on the shell socket.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I'm currently implementing it but:

one thread polling the shell sockets and additional PAIR sockets for interthread communication.

There is only one shell socket and I'm using queues for inter-thread communication instead of sockets.

one thread for the main shell and one per subshell.

The main shell is in the main thread, each subshell has its own thread.

@davidbrochart
Copy link
Collaborator Author

davidbrochart commented Jun 14, 2022

In 0bde221 the processing of the main shell requests is done in the main thread, and the shell messages are received in a separate thread.
I didn't implement sending the replies through a queue yet, so it's still not thread-safe.

@davidbrochart
Copy link
Collaborator Author

I added a test in a64cf72 which checks that execution in a subshell is done in parallel with execution in the main shell.

@JohanMabille
Copy link
Contributor

Regarding the implementation with a new zmq channel, the advantages would be:

  • no usage of the Queue (which is a synchronization object and should be avoided if we follow the ZMQ coding guideline)
  • we could avoid a new shell_id field in the message, asking for a new shell would simply drop a new connection file, with everything identical to the already existing connection file but the new shell port number.

@davidbrochart davidbrochart force-pushed the subshell branch 2 times, most recently from 66b038d to 6de6783 Compare June 15, 2022 11:07
@ianthomas23
Copy link
Collaborator

@davidbrochart I think this another PR that we can close as superseded by #1249.

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.

3 participants