-
Notifications
You must be signed in to change notification settings - Fork 947
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
fix: revert ipykernel dependency removal #3749
fix: revert ipykernel dependency removal #3749
Conversation
I think it is very important that we remove this dependency. Pulling ipykernel (which depends on e.g. Tornado) also makes the case of WebAssembly more cumbersome, as well as the usage of ipywidgets in xeus-python. I wish we removed it for 8.0. Could we move forward with removing the ipykernel dependency in ipywidgets? |
I think we should, let's try to get this removed for 8.1 and backport it to 7.x. The CI/test setup that got broken while trying to remove it in the first place could probably be easily solved by providing mock comms in this setup. |
Maybe an idea to chase the idea again of splitting into multiple (exactly pinned?) packages. This way, we don't hurt existing users (pip install ipywidgets, and it just works), and if you want to be thoughtful on what to install where you can. Maybe not the place to continue the discussion btw, but I cannot find the previous discussion on this topic. |
I don't think we should take such a complicated path. The initial issue that lead to reverting this change was merely test setups that were relying on ipykernel to be around through the ipywidgets dependency. I think having the comm package around, providing a default comm that fits the requirements for testing with ipywidgets, should be enough. |
So, if we include the comm dependency, no CI should break? Do you remember why we didn't do that before? |
Adding the comm dependency may be problematic because of the check here: ipywidgets/python/ipywidgets/ipywidgets/__init__.py Lines 27 to 30 in 39d3c5d
This check acts more or less as an ipykernel version check, without importing ipykernel. If we add the comm dependency we may break old ipykernel versions by not going into the except case.
The CI/tests break was due to having no-one registering a proper comm implementation in tests, because pytest runs are not done inside an ipykernel session, nor a xeus-python session. But you actually fixed this issue in ipython/comm@309b829 by providing a default comm implementation that does nothing (you basically implemented the "mock comms" I was suggesting above). So we may be able to remove again the ipykernel dependency with no other change. |
Prior to your change in the comm package, another way to workaround those CI/tests failure was by importing ipykernel in tests (what I suggest here #3729 (comment)), that way it would actually register a comm implementation that ipywidgets can use. |
If we make comm a dependency, we can remove the try/except. We could, however, see if ipykernel is in sys.modules, and if below a specific version, use ipykernel's Comm (basically the reverse of what we do now?) |
Yes that could be a good approach indeed. I'll come up with a PR. |
This change was too large for a fix release and hurt many people.
Let's move this to an 8.1 release, add comm as a dependency, and not have it break anything
see #3748 for the 7.x fix