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

Initial support for real time collaboration #109

Merged
merged 14 commits into from
Jul 12, 2021
Merged

Initial support for real time collaboration #109

merged 14 commits into from
Jul 12, 2021

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented May 28, 2021

References

#88

To try it out

https://jupyterlite--109.org.readthedocs.build/en/109/_static/lab/index.html?room=the-coolest-room

rtc-rtd.mp4

Code changes

  • Switch to WebrtcProvider
  • Set collaborative to true on the demo site
  • Basic link sharing to group users in rooms
  • Extra URL parameter to enable RTC? For example ?rtc=1 to enable, disabled otherwise -> not needed
  • Update the docs to show how to use RTC features

User-facing changes

Add options to enable RTC to end users.

Backwards-incompatible changes

@jtpio jtpio changed the title Use WebrtcProvider for RTC Initial support for real time collaboration May 28, 2021
constructor(options: IDocumentProviderFactory.IOptions) {
super(YJS_WEBSOCKET_URL, options.guid, options.ymodel.ydoc);
super(options.guid, options.ymodel.ydoc);
this.awareness = options.ymodel.awareness;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also make the signaling server(s) configurable via the settings / overrides.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this happen? I think we should add this configuration sprocket before merging.

@jtpio
Copy link
Member Author

jtpio commented May 28, 2021

A simple "room" support a la Whereby. So folks can share links that look like the following:

https://jupyterlite.readthedocs.io/en/latest/_static/lab/?room=the-coolest-room

image

@jtpio
Copy link
Member Author

jtpio commented May 28, 2021

RTC between lab and retro:

rtc-retro-lab.mp4

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jul 3, 2021

As mentioned on #207: it looks like there is some new behavior on the yjs connector that needs to be accounted for.

Not sure which of these we'd be most likely to land first...

@jtpio
Copy link
Member Author

jtpio commented Jul 8, 2021

Not sure which of these we'd be most likely to land first...

This PR still need a couple of fixes (sync when opening the documents), so probably we can get #207 in first and then rebase here.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

From https://readthedocs.org/projects/jupyterlite/builds/14201215/:

Command killed due to excessive memory consumption

Maybe a sign that we are starting to reach the limits? Will restart the build to double check.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

Also adding some docs about RTC: https://jupyterlite--109.org.readthedocs.build/en/109/rtc/index.html

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

Pushed a couple of improvements to fix the issue with duplicated content when a document is opened.

Now hitting that one when creating a new notebook that is already opened and edited by a peer (they need to choose Overwrite):

image

But it looks like it's fine when opening a existing example notebook, so maybe something to check in the jupyterlite contents manager.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

so probably we can get #207 in first and then rebase here.

Since RTC is opt-in and also requires setting the room query string parameter, maybe we can actually merge this one first.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

@bollwyvl would you like to give it a try on the RTD demo?

https://jupyterlite--109.org.readthedocs.build/en/109/_static/lab/index.html?room=the-coolest-room

Thinking we could ship as is if it works ok, and we can address the contents manager issue in follow-up PRs.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

we can address the contents manager issue

For that we might want to start using y-indexeddb as an alternative (configurable) backend for the contents manager: https://github.com/yjs/y-indexeddb

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jul 9, 2021

Command killed due to excessive memory consumption

Yeah, mamba let us sneak by over the initial hump, but webpack will increasingly destroy us.

Some band-aids until we really go after #118 (almost started trying to make some part of core federated, but worked on #238, because users can use it)

  • set the --max-old-space-size intentionally low, turn on all the caching, and hope that it can re-use cache results from failed runs to get there... eventually
  • split up some more parts of each app's build... perhaps the server and client could be separate builds... and heck, maybe server should be running in a webworker anyway...

using y-indexeddb as an alternative (configurable) backend for the contents

sounds like another issue... i don't understand fully the ramifications, but localForage is just nice, not criticial, as if a user's browser doesn't have indexeddb, they're likely going to have... other problems with this site.

In addition to the `collaborative` flag, end users must specify the `room` query
parameter in the URL. An example of such URL is as follows:

[https://jupyterlite.readthedocs.io/en/latest/\_static/lab/index.html?room=my-custom-room](https://jupyterlite.readthedocs.io/en/latest/_static/lab/index.html?room=my-custom-room)
Copy link
Collaborator

@bollwyvl bollwyvl Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wouldn't even make this a clickable link... and frankly, we shouldn't even suggest a room name, as we basically don't want people going to The One Room because it will get filled up with spam.

A nice approach might be to offer an RTC link generator with an in-page widget... though StringConcatenate isn't a thing, and wxyz doesn't work with requirejs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We can still show to specify the room parameter, but it could be something else or just a random string.

A nice approach might be to offer an RTC link generator with an in-page widget

Yes that would make it much more convenient to share a link.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An in-app one would be clutch, too, but could be a federated extension (let's not make webpack any worse until we make it better).

In this specific instance, I mean a link generator on the docs page, akin to the binder one, where we can hoist all of the bits and bobs.


- the host, for example `jupyterlite.example.com` or `myserver:5000`
- the name of the room parsed from the query string parameter, for example
`my-custom-room`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the actual key that gets used in the code on the public server could at least be obfuscated, e.g. by taking the hash of the appName, appVersion and the user-provided key. My confidence is very low in any of these things working between different deployments... and see spam.

Copy link
Member Author

@jtpio jtpio Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is still the risk of folks "seeing" each other. So having some kind of seed per deployment would greatly reduce the collisions.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jul 9, 2021

It indeed works (between browsers) on RTD, quite magical ✨

I feel like the presence indicator on the cursors b1 feels waaay better... and we'll want to make the list of "anonymous" names configurable, as people get real attached to those.

Overall (and i've raised this in core, too, somewhere) I feel like we need something that says, Hey buddy, you're broadcasting to everyone with this same room, and there's 800 people listening to your Untitled.ipynb, so don't put anything stupid in there.

Some ideas (not mutually exclusive):

  • overlays
    • i despise the dialogs (here, lemme take away your agency), but feel like having an opt-out "THIS IS SHARED" dialog might be appropriate here may be appropriate
    • we could use toast
  • main menu
    • top-right, with a distinctive icon, and a peer count if we can get it
  • file manager
    • change the file manager icon to be "shared folder" or "drive upload folder", make it orange
    • somehow indicate that every damn file is shared, so can be overwritten at any moment
  • launcher
    • add a note to top of launcher, e.g. "Shared with "
    • add "Shared" to all of the launcher category titles
  • dock panel
    • each shared tab should get something, but this is crazy lumino vdom
  • status bar
    • add a positive (and negative) indicator that you are sharing (with a link to the docs section if not sharing)

So, technically I see no issues to merging, but there are a lot of social and perceptual issues that make this complicated.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jul 9, 2021

In punching it a few different ways, i was able to get to a state where one of the windows (the second launched) couldn't make a new file. After the first started a new file, then the second window could make a new file.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jul 9, 2021

editing bar.vl.json and seeing it update the plot on the other screen is very satisfying.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

sounds like another issue... i don't understand fully the ramifications, but localForage is just nice, not criticial, as if a user's browser doesn't have indexeddb, they're likely going to have... other problems with this site.

Definitely something else for later. localForage is really nice indeed, but if we start seeing more issues with decentralized content, if one day we enable rtc by default, then maybe indexeddb backed by Yjs could be interesting.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

Some ideas (not mutually exclusive):

Nice set of ideas 👍 We can definitely track them in separate issues, and many of these might actually be about reusing existing plugins or extensions made for "normal" RTC.

In punching it a few different ways, i was able to get to a state where one of the windows (the second launched) couldn't make a new file. After the first started a new file, then the second window could make a new file.

Right I've also seen these. Not sure how frequent they happen but probably something to do the with the requestInitialContent logic.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jul 9, 2021

enable rtc by default

let's not, like ever, until the sharing feature upstream isn't entirely opaque to the user. Like, a different IDrive. with lots of indicators of who can see it. And identity.

@jtpio
Copy link
Member Author

jtpio commented Jul 9, 2021

let's not, like ever, until the sharing feature upstream isn't entirely opaque to the user. Like, a different IDrive. with lots of indicators of who can see it. And identity.

Yeah this will also be definitely relevant in lab and hub.

@jtpio jtpio marked this pull request as ready for review July 9, 2021 14:24
Copy link
Collaborator

@bollwyvl bollwyvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we can figure other stuff out in follow-ons.

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