Skip to content

Emit events for collaborative sessions#139

Merged
hbcarlos merged 14 commits intojupyterlab:mainfrom
hbcarlos:rtc-events
Apr 28, 2023
Merged

Emit events for collaborative sessions#139
hbcarlos merged 14 commits intojupyterlab:mainfrom
hbcarlos:rtc-events

Conversation

@hbcarlos
Copy link
Copy Markdown
Member

Emits events during a collaborative session.

Adds a new lab plugin to log the events in the dev console of the client. This might be useful to debug issues from users that don't have access to the terminal where the server is running, for example, users of JupyterHub-based deployments.

Adds a new dialog to alert users that multiple rooms are accessing the same file.

@hbcarlos hbcarlos requested review from afshin and fcollonval April 21, 2023 15:39
@hbcarlos hbcarlos self-assigned this Apr 21, 2023
@github-actions
Copy link
Copy Markdown
Contributor

Binder 👈 Launch a Binder on branch hbcarlos/jupyter_collaboration/rtc-events

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 21, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (b46809d) 0.00% compared to head (b6a2440) 0.00%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #139   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          7       7           
  Lines        408     452   +44     
=====================================
- Misses       408     452   +44     
Impacted Files Coverage Δ
jupyter_collaboration/app.py 0.00% <0.00%> (ø)
jupyter_collaboration/handlers.py 0.00% <0.00%> (ø)
jupyter_collaboration/rooms.py 0.00% <0.00%> (ø)
jupyter_collaboration/utils.py 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment thread jupyter_collaboration/handlers.py Outdated
else:
self._emit(
"initialize",
"There is another collaborative session accessing the same file.\nThe synchronization between rooms is not supported and you might lose some of your progress.",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From here, I thought synchronization between rooms was supported. Could you clarify?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we should rather say that it is experimental.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The UX of synchronization between rooms is pretty bad. With a good connection, a noticeable delay makes it unpredictable.

When two users are editing the same file using different models (for example, a user editing a notebook using the notebook view and another one using the plain text editor), propagating the changes from user A in room A to user B in room B takes a few seconds (there is a 1s delay to save the document + 1s delay for polling the file assuming that the communication and load delay is 0). During those 2s, user B can write a sentence that will disappear without any notification or asking the user if they want to overwrite the content.

In addition, we use the default content manager to load and save documents. When loading or saving a notebook, the content manager throws an error if the notebook is invalid (so it doesn't load/save the content). If user A in room A is editing the notebook using the plain text view, room B (with user B using the notebook editor) will be updated only when room A saves a valid notebook. Suppose user A forgets a curly bracket (which often happens because there is no visual indicator). In that case, everything user B is typing will be deleted once user A fixes the notebook without allowing user B to overwrite what is on the disk. Furthermore, if user B doesn't realize that their changes are not being saved, they can be typing for hours, and everything will be deleted.

The combination of auto-saving and room synchronization can be catastrophic; in my opinion, we can not call that "supported".

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fair enough, I agree that synchronization happening "through the file system" is not ideal, but we need to have at least a way to propagate changes between views. Otherwise, if the frontend reacts to file changes made in the backend, and you have one user with a notebook opened as a notebook document, and another user with the same notebook opened as a JSON document, what is the behavior?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Otherwise, if the frontend reacts to file changes made in the backend, and you have one user with a notebook opened as a notebook document, and another user with the same notebook opened as a JSON document, what is the behavior?

Yes, that's why I'm not removing the sync between rooms. Instead, I notify users saying that it is dangerous to use it.

Ideally, a secondary communication channel would be created to allow users to intervene in those decisions. I'm on it as well.

Copy link
Copy Markdown
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot @hbcarlos this is a great addition.

I have minor wording comments and two suggestions:

  • Use an enum for the level of message
  • Use the JupyterLab logger instead of the browser console

Comment thread jupyter_collaboration/events/session.yaml Outdated
Comment thread jupyter_collaboration/events/session.yaml Outdated
Comment thread jupyter_collaboration/events/session.yaml Outdated
Comment thread jupyter_collaboration/events/session.yaml Outdated
Comment thread jupyter_collaboration/events/session.yaml Outdated
Comment thread packages/collaboration-extension/src/filebrowser.ts Outdated
Comment thread packages/collaboration-extension/src/filebrowser.ts Outdated
Comment thread packages/collaboration-extension/src/filebrowser.ts Outdated
Comment thread packages/collaboration-extension/src/filebrowser.ts Outdated
Comment thread packages/collaboration-extension/src/filebrowser.ts Outdated
Comment thread jupyter_collaboration/events/session.yaml Outdated
Comment thread jupyter_collaboration/events/session.yaml Outdated
Comment thread jupyter_collaboration/events/session.yaml Outdated
hbcarlos and others added 3 commits April 26, 2023 15:24
Co-authored-by: Afshin Taylor Darian <git@darian.email>
Co-authored-by: Afshin Taylor Darian <git@darian.email>
Co-authored-by: Afshin Taylor Darian <git@darian.email>
@fcollonval fcollonval added enhancement New feature or request and removed maintenance labels Apr 27, 2023
Copy link
Copy Markdown
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @hbcarlos

Some final minor suggestions from me and it should be ready.

Comment thread package.json Outdated
Comment thread packages/collaboration-extension/src/filebrowser.ts Outdated
Comment thread packages/collaboration-extension/src/filebrowser.ts Outdated
@afshin
Copy link
Copy Markdown
Member

afshin commented Apr 27, 2023

cf. (for linking or if others are interested) the JEP for publishing versioned schema files: jupyter/enhancement-proposals#108

Copy link
Copy Markdown
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @hbcarlos

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.

5 participants