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

Comms support #18

Closed
martinRenou opened this issue Apr 8, 2021 · 15 comments
Closed

Comms support #18

martinRenou opened this issue Apr 8, 2021 · 15 comments
Projects
Milestone

Comments

@martinRenou
Copy link
Member

How easy do you think it would be to add comms support?

@martinRenou
Copy link
Member Author

martinRenou commented Apr 8, 2021

If we can implement comms, ipywidgets might work for free?

@jtpio
Copy link
Member

jtpio commented Apr 8, 2021

I think @ibdafna was looking into it at some point and it should be feasible in theory.

It's possible that the main issue for now is the dependency on ipykernel: https://github.com/jupyter-widgets/ipywidgets/blob/e0d41f6f02324596a282bc9e4650fd7ba63c0004/setup.cfg#L38

So having a lighter ipywidgets which could be a fork, or maybe by splitting parts of ipywidgets into several packages might work.

@martinRenou
Copy link
Member Author

martinRenou commented Apr 8, 2021

Indeed the ipykernel dependency might be annoying. The ipykernel dependency is because it provides the Python Comm implementation.

We had this issue in xeus-python as well, and we had to monkey patch ipykernel to inject our own Comm class (see this code). This might be needed in this pyodide case as well, although it is not a nice solution. I started a discussion here: jupyter-xeus/xeus-python#342 to hopefully find a solution to have a cleaner way for the "current" kernel to expose the comms implementation. Maybe using entry-points might be the key.

@bollwyvl
Copy link
Collaborator

One thing to consider: while we don't want to bite off re-implementing the entire widget stack, comms will be useful in many kernels. For example, we've considered replacing/augmenting bespoke websockets with comms for delivering LSP https://github.com/krassowski/jupyterlab-lsp/pull/278.

So: having this at the base js kernel level, and offering something that spans the inevitable webworker divide, with an initial in-worker python implementation, would be an interesting example of getting more features into more kernels.

It seems like basing it on the [ipykernel CommManager][https://github.com/ipython/ipykernel/blob/master/ipykernel/comm/manager.py#L17) isn't the worst idea.

@psychemedia
Copy link
Contributor

psychemedia commented Jun 3, 2021

Re: ipywidgets, I just started trying to go through the dependencies required to install it and get stuck with a missing core package (termios):

await micropip.install('https://files.pythonhosted.org/packages/f6/7d/3ecb0ebd0ce8dcdfa7bd47ab85c1d4a521e6770ef283d0824f5804994dfe/traitlets-5.0.5-py3-none-any.whl')
await micropip.install('https://files.pythonhosted.org/packages/c0/34/abf879c9c8a282b2d06c880f3791ce91b25de1912bdcfa4a209afebe12f3/ipython-7.24.1-py3-none-any.whl')
await micropip.install('https://files.pythonhosted.org/packages/fa/bc/9bd3b5c2b4774d5f33b2d544f1460be9df7df2fe42f352135381c347c69a/ipython_genutils-0.2.0-py2.py3-none-any.whl')
await micropip.install('https://files.pythonhosted.org/packages/11/53/084940a83a8158364e630a664a30b03068c25ab75243224d6b488800d43a/ipywidgets-7.6.3-py2.py3-none-any.whl')
await micropip.install('https://files.pythonhosted.org/packages/a6/c9/be11fce9810793676017f79ffab3c6cb18575844a6c7b8d4ed92f95de604/Pygments-2.9.0-py3-none-any.whl')
await micropip.install('https://files.pythonhosted.org/packages/39/7b/88dbb785881c28a102619d46423cb853b46dbccc70d3ac362d99773a78ce/pexpect-4.8.0-py2.py3-none-any.whl')

import ipywidgets as widgets

# ModuleNotFoundError: No module named 'termios'

@ibdafna
Copy link
Member

ibdafna commented Jun 3, 2021

The termios dependency is at the systems level so I don't think we'll be able to go past that one in pure WASM. The most likely solution would be to have a lightweight fork of jupyter-client which implements the communication protocol without all the dependencies the full-fat version has.

@davidbrochart
Copy link

The most likely solution would be to have a lightweight fork of jupyter-client which implements the communication protocol without all the dependencies the full-fat version has.

https://github.com/davidbrochart/kernel_driver might be a good candidate.

@jtpio
Copy link
Member

jtpio commented Jun 7, 2021

Do we really need a jupyter client?

The messaging part (in TypeScript) of the kernel would forward messages to the Python kernel. Then we'll need the Comm implementation in the kernel, which could be its own package, loaded by default in pyolite: https://github.com/jtpio/jupyterlite/tree/main/packages/pyolite-kernel/py/pyolite

Actually this might also require more patching / mocking of other packages like ipython (https://github.com/jtpio/jupyterlite/issues/116#issuecomment-853346270), since ipywidgets calls get_ipython() in some places.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jun 7, 2021

I'm sure there will be reason enough to eventually support jupyter_client, but in the near term it should not be necessary. I'm really hoping out for a way forward through/next to conda-forge to get to a sustainable patch/build/distribute toolchain, but we can't wait for some of the heavy stuff, and indeed, everything directly network-related is kind of a waste of time without major hacking at the pyodide level.

I do like being able to rely on having access to a shell, so (IPython.)get_ipython gets my 👍! I think we just need to look at real AP access patterns in the wild to figure out what we need to patch in the near term. And, of course, any time we can write things that work with more client kernels than the pyodide-based one, the less work it is to add the next language. I'll see if i can get something quacking...

@bollwyvl
Copy link
Collaborator

from https://github.com/jtpio/jupyterlite/pull/141#discussion_r650876224

@martinRenou Now I am wondering if it's feasible and if it makes sense to reuse @bollwyvl 's JS implementation of the comms instead, we would need to expose those comms to the "Python process".

@jtpio In theory probably yes (or hopefully yes), if the JS implementations follow what ipykernel expects. This could be passed as a custom Python package from JavaScript: https://pyodide.org/en/stable/usage/faq.html#how-do-i-create-custom-python-packages-from-javascript

Right, I think we win if these two PRs (#141 and #145) can be harmonized, but prettied up, moved to some new packages...

packages/
  kernel/
    kernel.ts        # gets the comm(Open|Msg|Close)Msg handlers
  js-kernel/
    kernel.ts        # has new CommManager({kernel: this})
  py-kernel/src/
    worker.ts        # also needs CommManager in here...
    kernel.ts        # ... injects it here e.g. `import js; (await js.import(${COMM_URL})).CommManager`
    comm.ts        # ... or maybe a de-asyncing wrapper here
    py/.../
      patches.py     # sys.modules hacks, as on #145
  comms/src/
    comm.ts
    comm-manager.ts
    providers/
      memory.ts
      # TODO: localForage.ts
      # TODO: yjs.ts

In patches.py, there are probably some more pointy bits... At the pyolite/import js boundary we need it to pretty much be dumb JSON types and Arrays, but it should be feasible.

It's good we a couple robust kernels kernels to kick this against, and we should try to leverage this towards making it relatively easy to add Comm support to a novel kernel, either "in-loop," for future js-based ones (e.g. .ts) or comlink, etc. for future worker-based kernels (e.g. .jl).

@martinRenou
Copy link
Member Author

In theory probably yes (or hopefully yes), if the JS implementations follow what ipykernel expects. This could be passed as a custom Python package from JavaScript

Will this actually work? The Pyolite python code running in a web worker it might not be possible to pass the JS comm implementation.

@bollwyvl
Copy link
Collaborator

python code running in a web worker it might not be possible to pass the JS

At it's simplest, I'm imagining the following pattern [around here](python code running in a web worker it might not be possible to pass the JS ).

 const blob = new Blob([
    [
        `importScripts("${commManagerUrl}"});`,
        `// secretly this did: this.comm_manager = new CommManager()`,
        `importScripts("${pyodideUrl}");`,
         //...

Then from inside python, the implementation would be like:

import js
comm = js.jupyterlite.comm.Comm({"kernel": js.kernel })

Where that js.kernel is the comlink instance, on the other side of the house. I think there's a better way to phrase it in the context of one of those shiny new issues!

@bollwyvl
Copy link
Collaborator

@martinRenou can we start using https://github.com/ipython/comm?

@martinRenou
Copy link
Member Author

I think we can, I will still need to keep the ipykernel.comm mock around for some time though. We need library authors to start using comm.

@jtpio
Copy link
Member

jtpio commented Feb 14, 2024

Closing in favor of jupyterlite/pyodide-kernel#37

For reference, this should work out of the box in the xeus-python kernel now: emscripten-forge/recipes#602

@jtpio jtpio closed this as completed Feb 14, 2024
@jtpio jtpio added this to the Reference milestone Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
MVP
Backlog
Development

No branches or pull requests

6 participants