search

ipython / ipykernel

IPython Kernel for Jupyter
https://ipykernel.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
649 stars 367 forks source link

Remove control queue #1210

Closed ianthomas23 closed 8 months ago

ianthomas23 commented 9 months ago

Currently when the control channel receives a message on the control stream, it puts it in a tornado.queues.Queue, and messages are popped off the queue one a time to be handled using process_control. I don't think the queue is necessary as the ZMQ socket/stream queues messages itself.

This PR removes the control queue, simplifying the code. The functions _flush_control_queue and poll_control_queue are removed as there is no queue to flush or poll. I assume the latter is considered public by its naming convention, but I don't know if any downstream libraries will be relying on its existence.

There are other queues used for the control channel and debugger that I have not looked at yet.

I have ensured that only one control message is handled at a time, to preserve the current behaviour. This uses a boolean flag in dispatch_control. This is a little strange as the use of ZMQStream.on_recv allows us to concurrently handle multiple control messages that we then have to suppress. There is future work to be done here to perhaps use the ZMQ socket rather than the stream, and jupyter-server/jupyter_server#1362 and jupyter/jupyter_client#997 are relevant.

ianthomas23 commented 9 months ago

The tests pass for this except for the downstream ipyparallel tests which are failing in various abort scenarios. It looks to me that ipyparallel doesn't use a separate control channel thread and to support that here I will need to add extra code (if the shell and control channels share a thread) to force the control messages to be processed before the shell ones. This will bring back some of the previous _flush_control_queue functionality, but in a different way.

ianthomas23 commented 8 months ago

To get test_debugger.py to pass the minimum dependencies CI run I needed to increase the minimum version of pyzmq from 24 to 25. In turn this required a bump of jupyter-client and tornado minimum versions (jupyter/jupyter_client#915). Summary of min version changes:

Dependency Previous min version Current min version Release date of current min version
jupyter-client 6.1.12 8.0.0 2023-01-26
pyzmq 24.0.0 25.0.0 2023-01-12
tornado 6.1.0 6.2.0 2022-07-03

I don't know if these changes will be considered too aggressive too soon.

blink1073 commented 8 months ago

Hmm, perhaps instead I can cut a version of jupyter_client 6.x that supports pyzmq 25?

blink1073 commented 8 months ago

Or, perhaps we wait and fold this in to an ipykernel 7.x release with #1079?

davidbrochart commented 8 months ago

Or, perhaps we wait and fold this in to an ipykernel 7.x release with #1079?

I was thinking about that, but would this PR still make sense once #1079 is in?

ianthomas23 commented 8 months ago

I was thinking about that, but would this PR still make sense once #1079 is in?

I see that #1079 still uses a queue for the control channel but it is no longer a Tornado queue of course. So this probably isn't needed after that, but the idea of simplifying by removing the queue might still be worth looking at.

If #1079 is near merging then we should ignore this for now. But it looks like there is quite a lot of work to do for Windows and downstream projects?

davidbrochart commented 8 months ago

Sorry Ian if your work in this PR ends up not being merged, I was aware of #1079 of course but Steve picked it up recently and I don't know if it's close to being merged, although he seems to be progressing fast.

ianthomas23 commented 8 months ago

Sorry Ian if your work in this PR ends up not being merged, I was aware of #1079 of course but Steve picked it up recently and I don't know if it's close to being merged, although he seems to be progressing fast.

No problem. This has been my first attempt at a substantial change to ipykernel and even if not merged it has been a good learning experience to get used to the CI and in particular the impact on downstream projects.

blink1073 commented 8 months ago

I had a go at the 6.x branch of jupyter_client, but I don't think it is revive-able. I think we can merge this and consider main to be 7.x, and focus on folding in #1079 as well.