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.

Sign up for GitHub

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

Jump to bottom

Move JupyterLab's YDocWebSocketHandler here #901

Closed
davidbrochart opened this issue Jul 4, 2022 · 15 comments
Closed

Move JupyterLab's YDocWebSocketHandler here #901

davidbrochart opened this issue Jul 4, 2022 · 15 comments
Labels
enhancement

Comments

@davidbrochart
Copy link
Contributor

I also opened this issue in JupyterLab.

Problem

I think that the YDocWebSocketHandler is not specific to JupyterLab and deserves to live in jupyter-server core.
It is a generic websocket server that can talk to any websocket provider for the supported Y documents. JupyterLab is just one of them, but I can imagine e.g. a Python web client that would also have a websocket provider, such as ypy-websocket, to implement a Jupyter client in the terminal. In that case we wouldn't need to run JupyterLab in the back-end but only Jupyter Server.

Proposed Solution

Move ydoc_handler.py to jupyter-server.

Additional context

This is also needed if we want to implement a new kernels REST API in jupyter-server.
@davidbrochart davidbrochart mentioned this issue Jul 5, 2022
Closed
@Zsailer
Copy link
Member

Zsailer commented Jul 5, 2022

I would suggest we make this a Jupyter Server extension, rather than merge it into core.

Putting it into the core server is a much more opinionated move than keeping it separate and allowing Jupyter Server consumers to opt-in by installing+enabling a server extension. Unless we have a good reason for the websocket to be included in every running Jupyter Server, it's safer to make it a server extension.

In that case, I'm all for moving the ydoc_handler to a new repo in the jupyter-server organization.

@ellisonbg
Copy link
Contributor

ellisonbg commented Jul 5, 2022 via email

@davidbrochart
Copy link
Contributor Author

Unless we have a good reason for the websocket to be included in every running Jupyter Server, it's safer to make it a server extension.

I think that RTC is going to be a core component of Jupyter, and even that it will be the only supported mode. I think that continuing to support a non-collaborative code base and a collaborative code base is only going to bring complexity, we should realize that fact now.

@echarles
Copy link
Member

echarles commented Jul 6, 2022 •
edited
Loading

I have opinions on where the code should reside, but they are not strong, so I let you discuss and decide :)

I think that RTC is going to be a core component of Jupyter, and even that it will be the only supported mode. I think that continuing to support a non-collaborative code base and a collaborative code base is only going to bring complexity, we should realize that fact now.

I can understand the value and ease to have only one code base. However, I see RTC as an opt-in feature, meaning that the default setup should/could stay as non-RTC as it is now without the CRDT additions.

Opting-in should be as easy as possible, with easy and transparent migration paths for the server administrator. Same should apply to opt-out: easy and transparent paths.

@Zsailer
Copy link
Member

Zsailer commented Jul 6, 2022 •
edited
Loading

I think that RTC is going to be a core component of Jupyter

I totally agree. But I agree with @echarles, that it will likely be a "opt-in" core feature. There are (quite prominent) cases in our ecosystem where Jupyter's version of RTC isn't relevant (see my next comment).

even that it will be the only supported mode.

The RTC and the y-doc approach depends on having a notebook document/model in-memory for collaboration, right? How would this work in a kernel gateway context? These "kernels-as-a-service" approaches don't have a notebook document in-memory to broadcast changes made to a document—they only provide kernels; no "cells" or document. These types of services/servers rely on the kernel websocket API (request-response across the websocket), not a collaboration approach on an underlying y-document model on the server.

I think that continuing to support a non-collaborative code base and a collaborative code base is only going to bring complexity, we should realize that fact now.

I think the complexity is well understood here, but I don't think it's a compelling enough case to eliminate the kernel websocket approach. If the Jupyter ecosystem needs both, we should support both the best we can. If we build a way to make kernel gateway (and related projects) work with the ydoc-handler, then we might be able to merge these two APIs.

As an aside...

Making it a server extension doesn't imply that it isn't "core" (IMO). For example, the terminals service is a "core" feature of jupyter server, but we've (recently) made it a server extension to allow consumers of Jupyter server to opt-out if needed. I tend to define "core" components as codebases that are maintained by the "core" team members and live in jupyter-server organization, not specifically in the jupyter_server package.

@davidbrochart, is there a specific issue with making ydoc_handler a server extension in this organization?

@davidbrochart
Copy link
Contributor Author

@davidbrochart, is there a specific issue with making ydoc_handler a server extension in this organization?

No, I guess you're right, thanks for bringing a larger picture here!

@echarles
Copy link
Member

echarles commented Jul 6, 2022

Making it a server extension doesn't imply that it isn't "core".

have we just invented the extended core software architecture? or something like that?

@davidbrochart
Copy link
Contributor Author

I created a server extension: https://github.com/davidbrochart/jupyter_server_ydoc.
I can transfer it to the jupyter-server organization, if everyone agrees.

@echarles
Copy link
Member

+1 to transfer.

Looking at https://github.com/davidbrochart/jupyter_server_ydoc/blob/main/jupyter_server_ydoc/ydoc.py, I don't see the features merged in JupyterLab jupyterlab/jupyterlab#12600 that allow to persist in SQLite with SQLiteYStore.

@davidbrochart
Copy link
Contributor Author

No, jupyterlab/jupyterlab#12600 has not been accepted yet in JupyterLab. I will port this PR to jupyter_server_ydoc when/if it is transfered, and we'll continue the discussion there.

@echarles
Copy link
Member

echarles commented Jul 10, 2022 •
edited
Loading

I have read jupyterlab/jupyterlab#12600 as merged but indeed it is not. One option for jupyterlab would be to depend on jupyter-server with the future jupyter_server_ydoc installed and enabled. This would make jupyterlab/jupyterlab#12600 not needed. WDYT?

@davidbrochart
Copy link
Contributor Author

By "future jupyter_server_ydoc", do you mean including jupyterlab/jupyterlab#12600?
In any case, I would rather not see JupyterLab having "a fork" of jupyter_server_ydoc. I think we'll need jupyterlab/jupyterlab#12600 anyway since it fixes a bug, then we can iterate on where/how to save the YStore.

@echarles
Copy link
Member

I was meaning implementing jupyterlab/jupyterlab#12600 into https://github.com/davidbrochart/jupyter_server_ydoc

We can discuss options during the next jupyter server call if it works for you. To wrap up on my side, +1 to this issue Move JupyterLab's YDocWebSocketHandler here.

@ellisonbg
Copy link
Contributor

ellisonbg commented Jul 10, 2022 via email

@davidbrochart
Copy link
Contributor Author

Done: https://github.com/jupyter-server/jupyter_server_ydoc.

@kevin-bates kevin-bates mentioned this issue Jul 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add YDocWebSocketHandler davidbrochart/jupyter_server
4 participants
@ellisonbg @echarles @Zsailer @davidbrochart