jupyter / qtconsole

Jupyter Qt Console
https://qtconsole.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
418 stars 201 forks source link

Compatibility with ipykernel 7 #623

Open Carreau opened 3 weeks ago

Carreau commented 3 weeks ago

It looks like qtconsole is not compatible with ipykernel 7,

Unfortunately I've not been following the development, and the previous maintainer stepped down a few month ago, and I'm afraid of the 6.x and 7.x branch diverging.

I would love to get some help get the downstream testing passing on all projects that depends on ipykernel.

cf https://github.com/ipython/ipykernel/issues/1222

ccordoba12 commented 1 week ago

Thanks for opening this issue @Carreau. The only test failing is this one:

https://github.com/jupyter/qtconsole/blob/c0a1dca9e08f998bf88c79ce5af307404d60c3ba/qtconsole/tests/test_inprocess_kernel.py#L24-L31

and the problem seems to be that we're not awaiting in self.kernel_manager.start_kernel() here:

https://github.com/jupyter/qtconsole/blob/c0a1dca9e08f998bf88c79ce5af307404d60c3ba/qtconsole/tests/test_inprocess_kernel.py#L13-L17

given that that method was changed to be async in https://github.com/ipython/ipykernel/pull/1079

However, I tried to do that and setUp hangs indefinitely, which makes me think that perhaps that method shouldn't be async (nor maybe anything for the inprocess kernel).

@davidbrochart, could you take a look at this one given that you implemented https://github.com/ipython/ipykernel/pull/1079? Thanks!

davidbrochart commented 1 week ago

Since you need to await self.kernel_manager.start_kernel(), the setUp() method has to be async, right?

Carreau commented 1 week ago

I came to a similar conclusion;

So far I found that we need at least

diff --git a/qtconsole/tests/test_inprocess_kernel.py b/qtconsole/tests/test_inprocess_kernel.py
index a6f9d86..dbdb634 100644
--- a/qtconsole/tests/test_inprocess_kernel.py
+++ b/qtconsole/tests/test_inprocess_kernel.py
@@ -4,16 +4,21 @@
 # Distributed under the terms of the Modified BSD License.

 import unittest
-
+import inspect
 from qtconsole.inprocess import QtInProcessKernelManager
+import ipykernel

-class InProcessTests(unittest.TestCase):
+class InProcessTests(unittest.IsolatedAsyncioTestCase):

-    def setUp(self):
+    async def asyncSetUp(self):
         """Open an in-process kernel."""
         self.kernel_manager = QtInProcessKernelManager()
-        self.kernel_manager.start_kernel()
+        if ipykernel.version_info >= (7,):
+            await self.kernel_manager.start_kernel()
+        else:
+            self.kernel_manager.start_kernel()
         self.kernel_client = self.kernel_manager.client()

     def tearDown(self):

But did not have time to look much further into it.

Carreau commented 1 week ago

I think that is not sufficient as await self.kernel_manager.start_kernel() now wait indefinitely until the kernel receives a stop signal if my memory serves me well.

davidbrochart commented 1 week ago

Yes, your asyncSetUp() method has to be launched as a background task with e.g. task_group.start_soon(asyncSetUp).

Carreau commented 1 week ago

I dont' use much async, I have this so far:

https://github.com/Carreau/qtconsole/tree/async

but, it has other issues with the KernelClient saying that self.kernel is None and should not be, but I can't find where kernel is suppoed to be set.