Repository: ipython/ipykernel

Issue:
Error opening new comm
With the `6.19.x` line, i'm seeing this in [this gist](https://gist.github.com/bollwyvl/543d7bc10705dd4006494f21aa48ca92).

![image](https://user-images.githubusercontent.com/45380/208188419-dc6aa6b4-acf9-4785-9edb-b0d4508d186b.png)

Rolling back to `6.17.1` does not show this behavior. 

The `target_name: "comm"` is suspect. Having a further look, but this seemed like a useful minimum reproducer to explore further.

Discussion:
blink1073: ```python
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:
```python
  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
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 https://github.com/ipython/ipykernel/issues/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.

Patch:
diff --git a/.github/workflows/downstream.yml b/.github/workflows/downstream.yml
index ab1f103..57c8561 100644
--- a/.github/workflows/downstream.yml
+++ b/.github/workflows/downstream.yml
@@ -84,6 +84,41 @@ jobs:
           pip install -e ".[test]"
           python test_ipykernel.py
 
+  qtconsole:
+    runs-on: ubuntu-latest
+    timeout-minutes: 20
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+
+      - name: Setup Python
+        uses: actions/setup-python@v4
+        with:
+          python-version: "3.9"
+          architecture: "x64"
+
+      - name: Install System Packages
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y --no-install-recommends '^libxcb.*-dev' libx11-xcb-dev libglu1-mesa-dev libxrender-dev libxi-dev libxkbcommon-dev libxkbcommon-x11-dev
+      - name: Install qtconsole dependencies
+        shell: bash -l {0}
+        run: |
+          cd ${GITHUB_WORKSPACE}/..
+          git clone https://github.com/jupyter/qtconsole.git
+          cd qtconsole
+          ${pythonLocation}/bin/python -m pip install -e ".[test]"
+          ${pythonLocation}/bin/python -m pip install pyqt5
+      - name: Install Jupyter-Client changes
+        shell: bash -l {0}
+        run: ${pythonLocation}/bin/python -m pip install -e .
+
+      - name: Test qtconsole
+        shell: bash -l {0}
+        run: |
+          cd ${GITHUB_WORKSPACE}/../qtconsole
+          xvfb-run --auto-servernum ${pythonLocation}/bin/python -m pytest -x -vv -s --full-trace --color=yes qtconsole
+
   downstream_check: # This job does nothing and is only used for the branch protection
     if: always()
     needs:
@@ -92,6 +127,7 @@ jobs:
       - jupyter_client
       - ipyparallel
       - jupyter_kernel_test
+      - qtconsole
     runs-on: ubuntu-latest
     steps:
       - name: Decide whether the needed jobs succeeded or failed
diff --git a/ipykernel/comm/comm.py b/ipykernel/comm/comm.py
index 295232e..77f9f07 100644
--- a/ipykernel/comm/comm.py
+++ b/ipykernel/comm/comm.py
@@ -42,7 +42,7 @@ class BaseComm(comm.base_comm.BaseComm):
 
 
 # but for backwards compatibility, we need to inherit from LoggingConfigurable
-class Comm(traitlets.config.LoggingConfigurable, BaseComm):
+class Comm(BaseComm, traitlets.config.LoggingConfigurable):
     """Class for communicating between a Frontend and a Kernel"""
 
     kernel = Instance("ipykernel.kernelbase.Kernel", allow_none=True)  # type:ignore[assignment]
@@ -68,12 +68,16 @@ class Comm(traitlets.config.LoggingConfigurable, BaseComm):
     def _default_comm_id(self):
         return uuid.uuid4().hex
 
-    def __init__(self, *args, **kwargs):
-        # Comm takes positional arguments, LoggingConfigurable does not, so we explicitly forward arguments
+    def __init__(self, target_name='', data=None, metadata=None, buffers=None, **kwargs):
+        # Handle differing arguments between base classes.
+        kernel = kwargs.pop('kernel', None)
+        if target_name:
+            kwargs['target_name'] = target_name
+        BaseComm.__init__(
+            self, data=data, metadata=metadata, buffers=buffers, **kwargs
+        )  # type:ignore[call-arg]
+        kwargs['kernel'] = kernel
         traitlets.config.LoggingConfigurable.__init__(self, **kwargs)
-        # drop arguments not in BaseComm
-        kwargs.pop("kernel", None)
-        BaseComm.__init__(self, *args, **kwargs)
 
 
 __all__ = ["Comm"]
diff --git a/ipykernel/comm/manager.py b/ipykernel/comm/manager.py
index 6bf73ad..c11b562 100644
--- a/ipykernel/comm/manager.py
+++ b/ipykernel/comm/manager.py
@@ -9,7 +9,7 @@ import traitlets
 import traitlets.config
 
 
-class CommManager(traitlets.config.LoggingConfigurable, comm.base_comm.CommManager):
+class CommManager(comm.base_comm.CommManager, traitlets.config.LoggingConfigurable):
 
     kernel = traitlets.Instance("ipykernel.kernelbase.Kernel")
     comms = traitlets.Dict()
@@ -17,5 +17,5 @@ class CommManager(traitlets.config.LoggingConfigurable, comm.base_comm.CommManag
 
     def __init__(self, **kwargs):
         # CommManager doesn't take arguments, so we explicitly forward arguments
-        traitlets.config.LoggingConfigurable.__init__(self, **kwargs)
         comm.base_comm.CommManager.__init__(self)
+        traitlets.config.LoggingConfigurable.__init__(self, **kwargs)


Test patch:
diff --git a/ipykernel/tests/test_comm.py b/ipykernel/tests/test_comm.py
index f8b0764..405841f 100644
--- a/ipykernel/tests/test_comm.py
+++ b/ipykernel/tests/test_comm.py
@@ -1,3 +1,5 @@
+import unittest.mock
+
 from ipykernel.comm import Comm, CommManager
 from ipykernel.ipkernel import IPythonKernel
 
@@ -47,10 +49,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
 
     assert manager.get_comm(comm.comm_id) == comm
     assert manager.get_comm('foo') is None