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

fix: revert ipykernel dependency removal #3749

Merged

Conversation

maartenbreddels
Copy link
Member

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

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch maartenbreddels/ipywidgets/fix_ipykernel_dep_8.x

@SylvainCorlay
Copy link
Member

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?

@martinRenou
Copy link
Member

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.

@maartenbreddels
Copy link
Member Author

Maybe an idea to chase the idea again of splitting into multiple (exactly pinned?) packages.
e.g. ipywidgets-core that only includes the python code and has less dependencies (just traitlets and ipython?). Move ipywidgets into a meta package (no code, just dependencies) that depends on ipywidgets-core, ipykernel, widgetsnbextension and jupyterlab_widgets.

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.

@martinRenou
Copy link
Member

e.g. ipywidgets-core that only includes the python code and has less dependencies (just traitlets and ipython?). Move ipywidgets into a meta package (no code, just dependencies) that depends on ipywidgets-core, ipykernel, widgetsnbextension and jupyterlab_widgets.

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.

@maartenbreddels
Copy link
Member Author

So, if we include the comm dependency, no CI should break? Do you remember why we didn't do that before?

@martinRenou
Copy link
Member

martinRenou commented Jul 26, 2023

Adding the comm dependency may be problematic because of the check here:

try:
from comm import get_comm_manager
except ImportError:
def get_comm_manager():

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.

@martinRenou
Copy link
Member

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.

@maartenbreddels
Copy link
Member Author

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?)

@martinRenou
Copy link
Member

Yes that could be a good approach indeed. I'll come up with a PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants