Replace ipykernel dependency by the comm dependency #3811

martinRenou · 2023-07-26T14:07:30Z

As discussed in #3749

This will:

Use the comm implementation from ipykernel if it's available
Otherwise import the comm implementation from the new comm package (typically the case of xeus-python, or in a test runtime where there is no kernel)

github-actions · 2023-07-26T14:07:45Z

👈 Launch a binder notebook on branch martinRenou/ipywidgets/add_comm_dependency

martinRenou · 2023-07-26T14:27:08Z

CI failure is due to the Notebook 7 release

SylvainCorlay · 2023-07-26T21:25:35Z

python/ipywidgets/ipywidgets/widgets/widget.py

                def create_comm(**kwargs):
                    from ipykernel.comm import Comm

                    return Comm(**kwargs)
+            else:
+                from comm import create_comm


maartenbreddels · 2023-07-27T11:26:20Z

I'll test this today!

maartenbreddels · 2023-07-27T14:04:13Z

@martinRenou this was more difficult to get right, the comm module could be imported, and ipykernel count not (yet) be imported (as is the case for solara).

Also, by putting the same API in a shim module, I think it cleaned up the code a bit. What do you think?

martinRenou · 2023-07-27T14:07:15Z

python/ipywidgets/ipywidgets/comm.py

+        from ipykernel.comm import Comm
+
+        return Comm(*args, **kwargs)


I'd really like to get rid of this some day.

That way we can remove the nasty patches of ipykernel.comm in xeus-python.

why do you want to get rid of this? in xeus-python you don't need to fix this right?

Let's say a library imports ipykernel and we import it before ipywidgets, then we end up in this block, breaking xeus-python. So xeus-python needs to patch this import.

only for old ipykernels right? Do you think that will still happen?

I'd like this piece of code to basically stay around forever, so we better get it right! :)
We can also skip this if we can detect xeus-python, or some other condition to make this more robust? Do you make use of a 'fake' ipykernel module in xeus-python?

Only for old ipykernel yes.

I'd like this piece of code to basically stay around forever

Let's talk again about it in ten years? 😄

Do you make use of a 'fake' ipykernel module in xeus-python?

Yeah we mock the ipykernel module https://github.com/jupyter-xeus/xeus-python/blob/main/src/xinterpreter.cpp#L77 . We should keep this mock in xeus-python for supporting old ipywidgets anyway.

Happy to discuss this in ten years from now :)

martinRenou · 2023-07-27T14:08:20Z

Yes putting it all in a separate file makes sense. Thanks for looking into it!

I need to check that this works nicely with xeus-python.

martinRenou

Tested with xeus-python, it works nicely! Thanks @maartenbreddels

SylvainCorlay · 2023-07-28T08:30:41Z

🎉

martinRenou · 2023-07-28T08:40:37Z

meeseeksdev please backport to 7.x

lumberbot-app · 2023-07-28T08:40:46Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout 7.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 b3300207f35571e253c355175ef989c6d5be418d

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #3811: Replace ipykernel dependency by the comm dependency'

Push to a named branch:

git push YOURFORK 7.x:auto-backport-of-pr-3811-on-7.x

Create a PR against branch 7.x, I would have named this PR:

"Backport PR #3811 on branch 7.x (Replace ipykernel dependency by the comm dependency)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

… comm dependency

Backport PR #3811: Replace ipykernel dependency by the comm dependency

ipywidgets 8.1.0 has been released, which includes the change at jupyter-widgets/ipywidgets#3811. With this change, ipykernel is no longer a dependnecy of ipywidgets, so jupyer-sphinx must now depend on ipykernel directly rather than assume it will be installed as a transitive dependency of ipywidgets. Without this change, jupyter-sphinx tests will fail CI. This has additionally led to CI failures in some packages that depend on jupyter-sphinx but not ipykernel.

vincent-grosbois · 2023-08-02T10:45:38Z

hello, with the backport of this to 7.X, it means now that ipywidgets 7.X is not working anymore on python 3.6 (as the comm module will require traitlet >= 5, which require python >= 3.7). Is it intended that ipywidgets 7.X won't support python 3.6 ? thanks

martinRenou · 2023-08-02T11:30:51Z

Thanks for commenting.

We can probably loosen the Traitlets dependency in the comm package in a patch release. Would you like to open a PR for this?

vincent-grosbois · 2023-08-02T12:53:14Z

I don't think I'll have the time soon, but here's another proposal: ipython/comm#15

bollwyvl · 2023-08-03T03:46:18Z

This broke import ipywidgets in jupyterlite-pyodide-kernel as can be seen in e.g.

https://ipywidgets.readthedocs.io/en/stable/try/lab/index.html?path=Widget%20List.ipynb

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[2], line 1
----> 1 import ipywidgets as widgets

File /lib/python3.10/site-packages/ipywidgets/__init__.py:54
     51         return
     52     register_comm_target()
---> 54 _handle_ipython()

File /lib/python3.10/site-packages/ipywidgets/__init__.py:52, in _handle_ipython()
     50 if ip is None:
     51     return
---> 52 register_comm_target()

File /lib/python3.10/site-packages/ipywidgets/__init__.py:41, in register_comm_target(kernel)
     39 """Register the jupyter.widget comm target"""
     40 from . import comm
---> 41 comm_manager = comm.get_comm_manager()
     42 if comm_manager is None:
     43     return

File /lib/python3.10/site-packages/ipywidgets/comm.py:18, in get_comm_manager()
     17 def get_comm_manager():
---> 18     if requires_ipykernel_shim():
     19         ip = get_ipython()
     21         if ip is not None and getattr(ip, "kernel", None) is not None:

File /lib/python3.10/site-packages/ipywidgets/comm.py:11, in requires_ipykernel_shim()
      8 if "ipykernel" in sys.modules:
      9     import ipykernel
---> 11     version = ipykernel.version_info
     12     return version < (6, 18)
     13 else:

AttributeError: module 'ipykernel' has no attribute 'version_info'

Some ways forward:

add version_info to the shim
- last i checked we couldn't use comm directly for... some reason, and it has its own comm implementation
- but less duplication is good
add some other attribute to check
add a special case for 'pyodide' in sys.modules (wouldn't recommend)

As this isn't a part I've monkied with much, would appreciate some insight from those that have, but would happily do the PR.

Also, worrying about python 3.6, and frankly, 3.7, is getting a bit wearisome if it can lead to any problems for folks that are on currently supported pythons.

martinRenou · 2023-08-03T06:44:22Z

We've been discussing this in #3819

add version_info to the shim

I think that' the way to go for now, longer term we get rid of the shim and use the comm module only.

last i checked we couldn't use comm directly for... some reason

I'm surprised, using the comm module to expose the pyolite's implementation of the comms should be almost straightforward.

Also, worrying about python 3.6, and frankly, 3.7

What do you mean about this? Related, it has been raised that comm could not be installed on Python 3.6 due to a strict traitlets dependency, it has been fixed in comm 0.1.4 by loosening the traitlets dependency.

martinRenou · 2023-08-03T06:45:04Z

~~I'll work on a PR to update the shims AND make use of the comm module right now.~~

Well actually jupyterlite/pyodide-kernel#53 (comment)

martinRenou marked this pull request as draft July 26, 2023 14:09

Replace ipykernel dependency by the comm dependency

49e9036

martinRenou force-pushed the add_comm_dependency branch from 3a2fb0e to 49e9036 Compare July 26, 2023 14:14

Don't workaround ipykernel in tests

22462a9

Handle Notebook 7 in dev install script

dd250bf

martinRenou marked this pull request as ready for review July 26, 2023 14:38

martinRenou requested a review from maartenbreddels July 26, 2023 14:38

SylvainCorlay reviewed Jul 26, 2023

View reviewed changes

SylvainCorlay approved these changes Jul 26, 2023

View reviewed changes

add shim module

2ef7e16

martinRenou commented Jul 27, 2023

View reviewed changes

martinRenou commented Jul 28, 2023

View reviewed changes

SylvainCorlay merged commit b330020 into jupyter-widgets:main Jul 28, 2023
19 checks passed

martinRenou deleted the add_comm_dependency branch July 28, 2023 08:37

lumberbot-app bot added the Still Needs Manual Backport label Jul 28, 2023

martinRenou pushed a commit to martinRenou/ipywidgets that referenced this pull request Jul 31, 2023

Backport PR jupyter-widgets#3811: Replace ipykernel dependency by the…

b02d083

… comm dependency

martinRenou added a commit that referenced this pull request Jul 31, 2023

Merge pull request #3814 from martinRenou/backport-of-pr-3811-on-7.x

01cd1a8

Backport PR #3811: Replace ipykernel dependency by the comm dependency

garrison mentioned this pull request Aug 1, 2023

Add direct dependency on ipykernel jupyter/jupyter-sphinx#226

Merged

jtpio mentioned this pull request Aug 2, 2023

ipykernel error with ipywidgets 8.1 and jupyterlite #3819

Closed

bollwyvl mentioned this pull request Aug 3, 2023

Use the comm package jupyterlite/pyodide-kernel#37

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ipykernel dependency by the comm dependency #3811

Replace ipykernel dependency by the comm dependency #3811

martinRenou commented Jul 26, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023

martinRenou commented Jul 26, 2023

SylvainCorlay Jul 26, 2023

maartenbreddels commented Jul 27, 2023

maartenbreddels commented Jul 27, 2023

martinRenou Jul 27, 2023

maartenbreddels Jul 27, 2023

martinRenou Jul 27, 2023

maartenbreddels Jul 27, 2023

maartenbreddels Jul 27, 2023

martinRenou Jul 28, 2023 •

edited

Loading

maartenbreddels Jul 28, 2023

martinRenou commented Jul 27, 2023

martinRenou left a comment

SylvainCorlay commented Jul 28, 2023

martinRenou commented Jul 28, 2023

lumberbot-app bot commented Jul 28, 2023

vincent-grosbois commented Aug 2, 2023

martinRenou commented Aug 2, 2023

vincent-grosbois commented Aug 2, 2023

bollwyvl commented Aug 3, 2023

martinRenou commented Aug 3, 2023

martinRenou commented Aug 3, 2023 •

edited

Loading

Replace ipykernel dependency by the comm dependency #3811

Replace ipykernel dependency by the comm dependency #3811

Conversation

martinRenou commented Jul 26, 2023 • edited Loading

github-actions bot commented Jul 26, 2023

martinRenou commented Jul 26, 2023

SylvainCorlay Jul 26, 2023

Choose a reason for hiding this comment

maartenbreddels commented Jul 27, 2023

maartenbreddels commented Jul 27, 2023

martinRenou Jul 27, 2023

Choose a reason for hiding this comment

maartenbreddels Jul 27, 2023

Choose a reason for hiding this comment

martinRenou Jul 27, 2023

Choose a reason for hiding this comment

maartenbreddels Jul 27, 2023

Choose a reason for hiding this comment

maartenbreddels Jul 27, 2023

Choose a reason for hiding this comment

martinRenou Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

maartenbreddels Jul 28, 2023

Choose a reason for hiding this comment

martinRenou commented Jul 27, 2023

martinRenou left a comment

Choose a reason for hiding this comment

SylvainCorlay commented Jul 28, 2023

martinRenou commented Jul 28, 2023

lumberbot-app bot commented Jul 28, 2023

vincent-grosbois commented Aug 2, 2023

martinRenou commented Aug 2, 2023

vincent-grosbois commented Aug 2, 2023

bollwyvl commented Aug 3, 2023

martinRenou commented Aug 3, 2023

martinRenou commented Aug 3, 2023 • edited Loading

martinRenou commented Jul 26, 2023 •

edited

Loading

martinRenou Jul 28, 2023 •

edited

Loading

martinRenou commented Aug 3, 2023 •

edited

Loading