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

Remove ipykernel and IPython as dependencies #3514

Open
maartenbreddels opened this issue Jul 8, 2022 · 22 comments
Open

Remove ipykernel and IPython as dependencies #3514

maartenbreddels opened this issue Jul 8, 2022 · 22 comments

Comments

@maartenbreddels
Copy link
Member

Looking at how ipykernel and IPython are used, I wonder if we can remove them as dependencies.
I think in xeus-python, and in pyodide environments we do not use the real packages as they are on pypi (right @martinRenou?)

Also, I'm working on a package that uses ipywidgets outside of a Jupyter context, and I don't need or use ipykernel and IPython.

Could we remove them as dependencies are say they should be provided by the 'host environment'/'kernel'?

Related #1892

@jasongrout
Copy link
Member

Does our code still import ipython? If it is generic, then +1 on removing the dependency.

@jasongrout
Copy link
Member

jasongrout commented Jul 8, 2022

It looks like we import things directly from IPython and ipykernel. Is that part just not working for you?

@maartenbreddels
Copy link
Member Author

maartenbreddels commented Jul 8, 2022 via email

@martinRenou
Copy link
Member

👍🏽 👍🏽 👍🏽 on removing the ipykernel dependency, which will make it easier for the xeus-python and jupyterlite cases. For IPython I'm less opinionated because we use it in xeus-python and jupyterlite anyway.

IIRC we only need ipykernel in ipywidgets for the comms implementation. So xeus-python and jupyterlite have to monkey patch ipykernel to provide their own comm implementations. See https://github.com/jupyter-xeus/xeus-python/blob/master/src/xinterpreter.cpp#L77 and https://github.com/jupyterlite/jupyterlite/blob/main/packages/pyolite-kernel/py/ipykernel/ipykernel/comm.py.

I've been wondering about having a generic way for kernels to expose their Comm implementation for libraries to use. Maybe through entry points or similar: jupyter-xeus/xeus-python#342.

@maartenbreddels
Copy link
Member Author

Currently, if you install notebook en jupyter lab, you get ipykernel as a dependency. Which I think is also a bit odd, but maybe can be considered user-friendly. But that means that in the most common environments we already have ipykernel.

But, given that there are other options to use ipywidgets (xeus-python, *-lite, others planned!), I don't think we should have ipykernel as a dependency. We could maybe live a while with saying: 'The ipykernel function that we use in ipywidgets is the interface that we require others to implement'.

Maybe start by dropping ipykernel, and keep the IPython dependency for now?

@jasongrout
Copy link
Member

Previous discussion also at #3209

@jasongrout
Copy link
Member

I commented on @Carreau's approach he proposed at jupyter-xeus/xeus-python#342 (comment).

@jasongrout
Copy link
Member

jasongrout commented Jul 26, 2022

The only place I see that we have a hard dependency on ipykernel is where we actually import the Comm package:

from ipykernel.comm import Comm

We also have a softer dependency at

comm = Instance('ipykernel.comm.Comm', allow_none=True)
where we have the default comm class referencing ipykernel.

If we took one of the approaches discussed on jupyter-xeus/xeus-python#342, and ipykernel were changed to instead import this new comm package and register itself as a comm provider, we could import from the comm package instead of ipykernel and use whatever implementation was provided, rather than depending on ipykernel (and we could also set that traitlet default to the comm package Comm class). This would require upstream work in ipykernel first, though. Anyone up for it?

We also have an implicit dependency on at least the interface provided by ipykernel at

if kernel:
parent = None
if hasattr(kernel, "get_parent"):
parent = kernel.get_parent()
elif hasattr(kernel, "_parent_header"):
# ipykernel < 6: kernel._parent_header is the parent *request*
parent = kernel._parent_header
if parent and parent.get("header"):
self.msg_id = parent["header"]["msg_id"]
self.__counter += 1
, but I presume other kernels are providing that interface?

@martinRenou
Copy link
Member

This would require upstream work in ipykernel first, though. Anyone up for it?

I'd be happy to do it!

but I presume other kernels are providing that interface?

Yes, we had to implement those interfaces in xeus-python and jupyterlite's pyolite kernel. We could probably also improve these parts.

@maartenbreddels
Copy link
Member Author

@martinRenou my guess is that https://github.com/jupyterlite/jupyterlite/blob/main/packages/pyolite-kernel/py/ipykernel/ipykernel/comm.py can be the basis for that comm package right?

I'd like to have a similar comm implementation, one that does not use traitlets (the overhead is large when using a lot of widgets).

@martinRenou
Copy link
Member

Just FYI I'm currently working on this new comm package.

@Carreau
Copy link
Contributor

Carreau commented Aug 1, 2022

@martinRenou and @jasongrout you should both have received PyPI invite to be owner of comm (currently a dummy package), on PyPI.

@martinRenou
Copy link
Member

Thank you!

@Carreau
Copy link
Contributor

Carreau commented Aug 1, 2022

No problem, thanks to you for working on this.

@martinRenou
Copy link
Member

I created https://github.com/martinRenou/comm (should probably be moved to the IPython org). @jasongrout @maartenbreddels it'd be great if you could tell me what you think of the approach.

I pushed some PRs here and there to update ipykernel and xeus-python:
jupyter-xeus/xeus-python#548
ipython/ipykernel#973

And one in ipywidgets: #3533

@maartenbreddels
Copy link
Member Author

maartenbreddels commented Aug 1, 2022 via email

@martinRenou
Copy link
Member

martinRenou commented Aug 1, 2022

Thank you! I'm totally fine removing traitlets in the base implementation. Although, will this break some configuration options in ipykernel?

@maartenbreddels
Copy link
Member Author

maartenbreddels commented Aug 1, 2022 via email

@martinRenou
Copy link
Member

Indeed, I'll do this! Thank you for the review :)

@martinRenou
Copy link
Member

Done! ipython/comm@86c8447

@martinRenou
Copy link
Member

Comm is now a package under the IPython org https://github.com/ipython/comm (huge thanks to @ivanov for moving the repo!).

@manzt
Copy link
Contributor

manzt commented Feb 16, 2024

is ipython still a required dep with comm? / would it be possible to dynamically import and call to get_ipython(), only when necessary?

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

No branches or pull requests

5 participants