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

Compatibility with ipykernel 7 #623

Open
Carreau opened this issue Oct 15, 2024 · 6 comments
Open

Compatibility with ipykernel 7 #623

Carreau opened this issue Oct 15, 2024 · 6 comments

Comments

@Carreau
Copy link
Member

Carreau commented Oct 15, 2024

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 ipython/ipykernel#1222

@ccordoba12 ccordoba12 changed the title compatibility ipykernel 7 Compatibility with ipykernel 7 Oct 26, 2024
@ccordoba12
Copy link
Collaborator

ccordoba12 commented Oct 26, 2024

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

def test_execute(self):
"""Test execution of shell commands."""
# check that closed works as expected
assert not self.kernel_client.iopub_channel.closed()
# check that running code works
self.kernel_client.execute('a=1')
assert self.kernel_manager.kernel.shell.user_ns.get('a') == 1

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

def setUp(self):
"""Open an in-process kernel."""
self.kernel_manager = QtInProcessKernelManager()
self.kernel_manager.start_kernel()
self.kernel_client = self.kernel_manager.client()

given that that method was changed to be async in ipython/ipykernel#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 ipython/ipykernel#1079? Thanks!

@davidbrochart
Copy link
Member

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

@Carreau
Copy link
Member Author

Carreau commented Oct 28, 2024

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
Copy link
Member Author

Carreau commented Oct 28, 2024

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
Copy link
Member

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

@Carreau
Copy link
Member Author

Carreau commented Oct 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants