Skip to content

Rewrite as JupyterLab frontend plugin #49

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

Merged
merged 15 commits into from
May 19, 2025
Merged

Rewrite as JupyterLab frontend plugin #49

merged 15 commits into from
May 19, 2025

Conversation

ianthomas23
Copy link
Member

Work in progress to rewrite as a conventional JupyterLab frontend plugin following significant changes in JupyterLite to unify with JupyterLab (jupyterlite/jupyterlite#1590). Builds against prereleases of both JupyterLite and JupyterLab.

The Terminal itself works, but not the full shutdown and reopen functionality.

Quite a lot of this is necessarily reproduced from the equivalent JupyterLab code. It will be possible to reduce some of the duplication when jupyterlab/jupyterlab#17395 is merged and released.

@ianthomas23 ianthomas23 added the enhancement New feature or request label Mar 19, 2025
Copy link

vercel bot commented Mar 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jupyterlite-terminal ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 19, 2025 7:35am

@jtpio
Copy link
Member

jtpio commented Mar 21, 2025

Quite a lot of this is necessarily reproduced from the equivalent JupyterLab code. It will be possible to reduce some of the duplication when jupyterlab/jupyterlab#17395 is merged and released.

JupyterLite should now have these changes via jupyterlite/jupyterlite#1597.

And available in the latest pre-release: https://github.com/jupyterlite/jupyterlite/releases/tag/v0.6.0a5

@ianthomas23
Copy link
Member Author

@jtpio The changes in Lab are really useful. Instead of writing my own LiteTerminalManager class I can reuse Lab's TerminalManager and pass it my own ITerminalAPIClient. However, there is a problem in TerminalManager.connectTo at https://github.com/jupyterlab/jupyterlab/blob/85c82eba1caa7e28a0d818c0840e13756c1b1256/packages/services/src/terminal/manager.ts#L123

This creates an instance of TerminalConnection which is not what I want as it attempts to create a websocket, etc, in the default manner. Instead I want to create an instance of my own LiteTerminalConnection. So I think there needs to be another change in Lab, perhaps adding a new createTerminal function to ITerminalAPIClient that is called from TerminalManager.connectTo.

@jtpio
Copy link
Member

jtpio commented Mar 27, 2025

This creates an instance of TerminalConnection which is not what I want as it attempts to create a websocket, etc, in the default manner.

Were you able to try passing a custom ServerConnection.ISettings when instantiating the TerminalManager?

This sounds similar to the KernelManager with the KernelConnection (which creates the WebSocket). In JupyterLite we provide a custom WebSocket here: https://github.com/jupyterlite/jupyterlite/blob/f175a5d131b78a71103cf7e33947f03698df0755/packages/services-extension/src/index.ts#L134-L142

@jtpio
Copy link
Member

jtpio commented Mar 27, 2025

Arf, I think we need to pass the client to the TerminalConnection so it uses the provided client. It looks like it's indeed not the case currently in the lab code base.

We should likely do something similar to the KernelConnection: https://github.com/jupyterlab/jupyterlab/blob/85c82eba1caa7e28a0d818c0840e13756c1b1256/packages/services/src/kernel/default.ts#L54-L56

@jtpio
Copy link
Member

jtpio commented Mar 27, 2025

@ianthomas23 would you like to open the PR, to replace the use of the default Rest API here? https://github.com/jupyterlab/jupyterlab/blob/85c82eba1caa7e28a0d818c0840e13756c1b1256/packages/services/src/terminal/default.ts#L232

@jtpio
Copy link
Member

jtpio commented Mar 27, 2025

OK I opened jupyterlab/jupyterlab#17437 to fix it, so it can be merged for 4.4.0.

@ianthomas23
Copy link
Member Author

Fix usage of ITerminalAPIClient in TerminalConnection jupyterlab/jupyterlab#17437

Thanks, I have confirmed that that PR fixes the problem for me here.

I won't push any more changes here until there are JupyterLab and Lite prereleases containing the fix.

@jtpio
Copy link
Member

jtpio commented Apr 2, 2025

I won't push any more changes here until there are JupyterLab and Lite prereleases containing the fix.

FYI JupyterLab 4.4.0rc1 is out (with the upstream fix): https://github.com/jupyterlab/jupyterlab/releases/tag/v4.4.0rc1

I'll look into updating notebook and lite, so we can update the PR here too.

@ianthomas23
Copy link
Member Author

This is now working using jupyterlab 4.4.0 and jupyterlite-core 0.6.0a6. Terminal shutdown works both from the UI and using the exit command in the terminal. It doesn't yet support:

  • Reopening a closed but not shutdown terminal tab
  • Using the terminal settings for closing and shutdown

but neither of these work in the main branch yet anyway so they are not regressions and don't need to slow down the release plan for JupyterLite 0.6.0.

@ianthomas23
Copy link
Member Author

@jtpio This is now using jupyterlab 4.4.0, jupyterlite-core 0.6.0a9 and cockle 0.0.19. To get the browsingContextId the terminal plugins tries to use the IServiceWorkerManager

optional: [IServiceWorkerManager],

following how it is done in the pyodide-kernel. But this isn't working for me, in fact the app isn't even defined. This is what I see locally and in the vercel deployment here:

Screenshot 2025-04-22 at 14 18 06

I note that the pyodide kernel works fine in the vercel deployment, so maybe this is something to do with the order of initialisation of the plugins?

@jtpio
Copy link
Member

jtpio commented Apr 22, 2025

But this isn't working for me, in fact the app isn't even defined

The app is null because the plugin activation happens during the first phase, when loading the ServiceManagerPlugin: https://jupyterlab.readthedocs.io/en/latest/extension/extension_dev.html#service-manager-plugins

Regarding the missing IServiceWorkerManager, could it be because the current jupyterlab config in the package.json does not defined the sharedPackages? For example for the Pyodide kernel it looks like the following:

https://github.com/jupyterlite/pyodide-kernel/blob/309c7bf5824bf680420937472b228f31505be77b/packages/pyodide-kernel-extension/package.json#L74-L77

@ianthomas23
Copy link
Member Author

It turns out that specifying sharedPackages isn't required.

Instead the solution is to have two separate plugins, the first is a ServiceManagerPlugin to provide the LiteTerminalManager without requiring any extra plugin dependencies (because they aren't available at that time). The second is a normal JupyterFrontEndPlugin that gets the browsingContextId from the ServiceWorkerManager and passes it to the LiteTerminalManager. The only complexity here is the class of the terminalManager passed to the second plugin is definitely a LiteTerminalManager (it has just been created) but we can only guarantee it is a more general Terminal.IManager so I have added a type guard to check for the existence of the browserContextId setter before calling it.

@ianthomas23
Copy link
Member Author

Merging to make a pre-release.

@ianthomas23 ianthomas23 merged commit 24a17cd into main May 19, 2025
10 checks passed
@ianthomas23 ianthomas23 deleted the drop-server-app branch May 19, 2025 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants