Drop Tornado #876

davidbrochart · 2022-02-23T19:00:03Z

Closes #656

ipykernel/ipkernel.py

fcollonval

Thanks @davidbrochart for this. I got a question and a comment.

ipykernel/control.py

ipykernel/iostream.py

davidbrochart · 2022-02-24T13:31:31Z

Thanks @davidbrochart for this. I got a question and a comment.

Thanks for the review @fcollonval.
This is still a WIP so I'll make this PR a draft. I can see that the kernel is functional e.g. in JupyterLab, but there are still some issues, at least with shutdown.

davidbrochart · 2022-02-28T21:33:24Z

ipykernel/kernelbase.py


        self.log.debug('Stopping shell ioloop')
        shell_io_loop = self.shell_stream.io_loop
-        shell_io_loop.add_callback(shell_io_loop.stop)
+        # FIXME: shouldn't need to be threadsafe
+        shell_io_loop.call_soon_threadsafe(shell_io_loop.stop)


I thought that we shouldn't need to call_soon_threadsafe because the shell event loop runs in the main thread, but actually we do because we are processing the shutdown request from the control channel, which runs in another thread.
Correct me if I'm wrong 😄

I think you're right

davidbrochart · 2022-02-28T22:10:47Z

ipykernel/debugger.py

@@ -382,7 +382,7 @@ def start(self):
            self.session.send(self.shell_socket, 'execute_request', content,
                              None, (self.shell_socket.getsockopt(ROUTING_ID)))

-            ident, msg = self.session.recv(self.shell_socket, mode=0)
+            ident, msg = await self.session.async_recv(self.shell_socket, mode=0)


Will need jupyter/jupyter_client#749.

davidbrochart · 2022-02-28T22:12:06Z

ipykernel/iostream.py

-            # FIXME: original code was:
-            # self._io_loop.call_later(self.flush_interval, self._flush)
-            self._io_loop.call_soon_threadsafe(self._flush)
+            self._io_loop.call_soon_threadsafe(self._io_loop.call_later, self.flush_interval, self._flush)


Not sure if call_soon_threadsafe is needed. If not, we could keep the original code.

davidbrochart · 2022-02-28T22:13:05Z

ipykernel/kernelapp.py

@@ -367,7 +367,7 @@ def close(self):
            if socket and not socket.closed:
                socket.close()
        self.log.debug("Terminating zmq context")
-        # FIXME
+        # FIXME: this line breaks shutdown
        # self.context.term()


Not sure why terminating the ZMQ context creates issues here.

Maybe related: jupyter-server/jupyter_server#677 (comment)

Thanks @blink1073, I can see that currently, we don't close the heartbeat socket before terminating the ZMQ context, but the other way around: we wait for the context to be terminated before closing the socket.
Maybe this can be the root cause, @minrk ?

Context.term blocks until all sockets are closed and all unsent messages are delivered or discarded. A socket may close after the context if it is done from another thread. But it can't be done from a coroutine, for instance.

The things to check when a context hangs are always:

are there sockets that haven't been closed, and

was LINGER set on those sockets so they discard messages eventually

You can debug leftover sockets with self.context._sockets which is a set of weakrefs to sockets (this is private and subject to change, but works for debugging).

The heartbeat socket doesn't share this context, so I don't think that's relevant.

minrk · 2022-03-01T07:49:45Z

ipykernel/zmqstream.py

+    def send_multipart(self, msg_list, flags=0, copy=True, track=False, **kwargs):
+        return self.socket.send_multipart(msg_list, copy=copy)
+
+    def flush(self, flag=None, limit = None):


Flush has historically been very important behavior for ipykernel to behave appropriately. Making it a no-op doesn't seem great to me (It may not be as relevant today with everything being async, but this is a big change).

But flush seems to be supported only for Tornado's IOLoop and ZMQStream, right? I haven't found any reference of it outside of this context.

That's true, because send is async, but not awaitable with ZMQStream.send. We don't need this anymore as long as we await all our sends, since asyncio Socket.send is awaitable.

minrk · 2022-03-01T07:51:34Z

ipykernel/kernelbase.py


        self.log.debug('Stopping shell ioloop')
        shell_io_loop = self.shell_stream.io_loop
-        shell_io_loop.add_callback(shell_io_loop.stop)
+        # FIXME: shouldn't need to be threadsafe
+        shell_io_loop.call_soon_threadsafe(shell_io_loop.stop)


I think you're right

minrk · 2022-03-01T07:58:56Z

ipykernel/kernelapp.py

@@ -367,7 +367,7 @@ def close(self):
            if socket and not socket.closed:
                socket.close()
        self.log.debug("Terminating zmq context")
-        # FIXME
+        # FIXME: this line breaks shutdown
        # self.context.term()


Context.term blocks until all sockets are closed and all unsent messages are delivered or discarded. A socket may close after the context if it is done from another thread. But it can't be done from a coroutine, for instance.

The things to check when a context hangs are always:

are there sockets that haven't been closed, and

was LINGER set on those sockets so they discard messages eventually

You can debug leftover sockets with self.context._sockets which is a set of weakrefs to sockets (this is private and subject to change, but works for debugging).

The heartbeat socket doesn't share this context, so I don't think that's relevant.

davidbrochart · 2022-03-11T09:33:44Z

Although ipykernel seems functional with these changes, I don't really understand the test failures.
I think the next step is getting rid of the ZMQStream with their on_recv callbacks (which were really specific to Tornado), and just have tasks that receive messages and handle them. I would also like to get rid of the jupyter_client dependency. It seems weird for a "server" to depend on a "client".

minrk · 2022-03-11T19:27:35Z

I would also like to get rid of the jupyter_client dependency

I wouldn't do that. jupyter-protocol would have been a better name for that repo, since that's really what it defines. It is meant to be a dependency for any Python use of the Jupyter protocol, and that includes ipykernel.

minrk · 2022-03-11T19:36:55Z

I think the next step is getting rid of the ZMQStream with their on_recv callbacks

👍

a simpler while True: poll/recv/process/send coroutine ought to work, and might even make channel priority easier to ensure.

davidbrochart · 2023-01-25T09:43:07Z

Closing in favor of #1079.

davidbrochart added the enhancement label Feb 23, 2022

blink1073 reviewed Feb 23, 2022

View reviewed changes

ipykernel/ipkernel.py Outdated Show resolved Hide resolved

davidbrochart force-pushed the asyncio branch from f516a39 to 1e60a97 Compare February 23, 2022 21:34

fcollonval suggested changes Feb 24, 2022

View reviewed changes

ipykernel/control.py Outdated Show resolved Hide resolved

ipykernel/iostream.py Outdated Show resolved Hide resolved

ipykernel/iostream.py Outdated Show resolved Hide resolved

davidbrochart marked this pull request as draft February 24, 2022 13:31

davidbrochart force-pushed the asyncio branch from 6b4b0b5 to 5c6282f Compare February 24, 2022 14:12

davidbrochart mentioned this pull request Feb 28, 2022

Add session async recv jupyter/jupyter_client#749

Closed

davidbrochart commented Feb 28, 2022

View reviewed changes

minrk reviewed Mar 1, 2022

View reviewed changes

davidbrochart force-pushed the asyncio branch from 3ab9a87 to e3a6c9f Compare March 7, 2022 21:23

davidbrochart force-pushed the asyncio branch 2 times, most recently from 0b9ddbd to 76ee4ac Compare April 7, 2022 09:31

davidbrochart force-pushed the asyncio branch from 76ee4ac to 8dabab3 Compare June 20, 2022 08:05

Drop Tornado

dfa991e

davidbrochart force-pushed the asyncio branch from 8dabab3 to dfa991e Compare June 20, 2022 08:49

davidbrochart mentioned this pull request Jan 25, 2023

Replace Tornado with AnyIO #1079

Merged

davidbrochart closed this Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Tornado #876

Drop Tornado #876

davidbrochart commented Feb 23, 2022

fcollonval left a comment

davidbrochart commented Feb 24, 2022

davidbrochart Feb 28, 2022

minrk Mar 1, 2022

davidbrochart Feb 28, 2022

davidbrochart Feb 28, 2022

davidbrochart Feb 28, 2022

blink1073 Feb 28, 2022

davidbrochart Mar 1, 2022

minrk Mar 1, 2022

minrk Mar 1, 2022

davidbrochart Mar 1, 2022

minrk Mar 2, 2022 •

edited

Loading

minrk Mar 1, 2022

minrk Mar 1, 2022

davidbrochart commented Mar 11, 2022

minrk commented Mar 11, 2022

minrk commented Mar 11, 2022

davidbrochart commented Jan 25, 2023

Drop Tornado #876

Drop Tornado #876

Conversation

davidbrochart commented Feb 23, 2022

fcollonval left a comment

Choose a reason for hiding this comment

davidbrochart commented Feb 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minrk Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart commented Mar 11, 2022

minrk commented Mar 11, 2022

minrk commented Mar 11, 2022

davidbrochart commented Jan 25, 2023

minrk Mar 2, 2022 •

edited

Loading