Fix comms and add qtconsole downstream test

[blink1073]

I noticed qtconsole was failing in jupyter_client due to comms.

Fixes #1059

We had issues with multiple inheritance, which should be fixed in this PR.

[blink1073]

def test_kernel_to_frontend(self):
        """Communicate from the kernel to the frontend."""
        comm_manager = self.comm_manager
        blocking_client = self.blocking_client
    
        class DummyCommHandler():
            def __init__(self):
                comm_manager.register_target('test_api', self.comm_open)
                self.last_msg = None
    
            def comm_open(self, comm, msg):
                comm.on_msg(self.comm_message)
                comm.on_close(self.comm_message)
                self.last_msg = msg['content']['data']
                self.comm = comm
    
            def comm_message(self, msg):
                self.last_msg = msg['content']['data']
    
        handler = DummyCommHandler()
        blocking_client.execute(
        "from ipykernel.comm import Comm\n"
        "comm = Comm(target_name='test_api', data='open')\n"
        "comm.send('message')\n"
        "comm.close('close')\n"
        "del comm\n"
        "print('Done')\n"
        )
        # Get input
        msg = self._get_next_msg()
        assert msg['header']['msg_type'] == 'execute_input'
        # Open comm
        msg = self._get_next_msg()
        assert msg['header']['msg_type'] == 'comm_open'
        comm_manager._dispatch(msg)
>       assert handler.last_msg == 'open'
E       AssertionError: assert None == 'open'
E        +  where None = <qtconsole.tests.test_comms.Tests.test_kernel_to_frontend.<locals>.DummyCommHandler object at 0x7fad6246eb80>.last_msg

qtconsole/tests/test_comms.py:89: AssertionError
------------------------------ Captured log call -------------------------------
ERROR    traitlets:comms.py:[134](https://github.com/jupyter/jupyter_client/actions/runs/3660730825/jobs/6188413300#step:7:135) No such comm target registered: comm

[blink1073]

cc @maartenbreddels

[maartenbreddels]

hmm, I don't understand this test. The comm_open should only be triggered when it's in opened in say a frontend, not when directly created.

[maartenbreddels]

hmm, qtconsole has its own comm manager.

[blink1073]

Ah, so qtconsole, being another application, would have to define comm.create_comm and comm.get_comm_manager?

[ccordoba12]

Ah, so qtconsole, being another application, would have to define comm.create_comm and comm.get_comm_manager?

Yep, that's the idea. We're using comms in Spyder to define a lot of interactions between kernel and frontend, e.g. retrieving the value of a variable to display it in our specialized viewers, modifying sys.path from the frontend, interrupting the kernel to update the Variable Explorer while a computation is running, etc.

[blink1073]

@maartenbreddels I believe this also fixes #1059

[maartenbreddels]

I didn't understand what went wrong, and what, so looking at the stracktrace:

  File "/Users/maartenbreddels/github/clean-env/lib/python3.8/site-packages/ipywidgets/widgets/widget.py", line 528, in open
    self.comm = Comm(**args)
  File "/Users/maartenbreddels/github/clean-env/lib/python3.8/site-packages/ipykernel/comm/comm.py", line 73, in __init__
    traitlets.config.LoggingConfigurable.__init__(self, **kwargs)
  File "/Users/maartenbreddels/github/clean-env/lib/python3.8/site-packages/traitlets/config/configurable.py", line 85, in __init__
    super().__init__(**kwargs)
  File "/Users/maartenbreddels/github/clean-env/lib/python3.8/site-packages/traitlets/traitlets.py", line 1370, in __init__
    super().__init__(*super_args, **super_kwargs)

Which revealed that the BaseComm ctor was being called twice. To test this, I've added this test locally:

diff --git a/ipykernel/tests/test_comm.py b/ipykernel/tests/test_comm.py
index f8b0764..fe2bfdb 100644
--- a/ipykernel/tests/test_comm.py
+++ b/ipykernel/tests/test_comm.py
@@ -1,6 +1,6 @@
 from ipykernel.comm import Comm, CommManager
 from ipykernel.ipkernel import IPythonKernel
-
+import unittest.mock
 
 def test_comm(kernel):
     manager = CommManager(kernel=kernel)
@@ -30,6 +30,7 @@ def test_comm_manager(kernel):
     manager = CommManager(kernel=kernel)
     msgs = []
 
+
     def foo(comm, msg):
         msgs.append(msg)
         comm.close()
@@ -47,10 +48,12 @@ def test_comm_manager(kernel):
     manager.register_target("fizz", fizz)
 
     kernel.comm_manager = manager
-    comm = Comm()
-    comm.on_msg(on_msg)
-    comm.on_close(on_close)
-    manager.register_comm(comm)
+    with unittest.mock.patch.object(Comm, "publish_msg") as publish_msg:
+        comm = Comm()
+        comm.on_msg(on_msg)
+        comm.on_close(on_close)
+        manager.register_comm(comm)
+        assert publish_msg.call_count == 1

Which fails on the main branch, and passes on this branch. (can you apply this diff?)

I still don't understand how this can cause #1059 to now show the ipywidgets. I'll go into that issue to explore that.

[blink1073]

Thanks, I applied the diff.

[blink1073]

I think the console error was from a duplicate call to the constructor, where target_name was set to comm. I was able to see that locally when testing.

[bollwyvl]

LGTM! fixes the issue on #1059 (link to binder gist at the top pointing to this PR).

[maartenbreddels]

Which revealed that the BaseComm ctor was being called twice. To test this, I've added this test locally:

Indeed, I agree with that @blink1073

@bollwyvl great to hear that.

Still feel slightly uncomfortable with not understanding how I still get to see a widget, and you did not.

[maartenbreddels]

Still feel slightly uncomfortable with not understanding how I still get to see a widget and you did not.

Actually, I don't think you explicitly said this. Did you still see a widget @bollwyvl ?

[blink1073]

I did not, before the fix.

[bollwyvl]

At no point were all widgets not showing, but every one would trigger a couple browser error messages, which would very rapidly stack up.

In the scope of some automated stress test demos with thousands of widgets (some created on the client #3653), some of them would not appear... even accounting for the registry issue, rolling back to 6.17 made my tests pass.

[maartenbreddels]

some automated stress test demos with thousands of widgets

is this public / open source?