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

Make the use of YStore configurable #430

Open
dlqqq opened this issue Jan 16, 2025 · 2 comments
Open

Make the use of YStore configurable #430

dlqqq opened this issue Jan 16, 2025 · 2 comments
Labels
enhancement New feature or request

Comments

@dlqqq
Copy link
Member

dlqqq commented Jan 16, 2025

Problem

By default, jupyter_server_ydoc persists every Yjs update made by every client connected to the server. These are stored in .jupyter_ystore.db by the SQLiteYStore class (defined in pycrdt-websocket). This feature allows a document's edits to be "replayed" from its origin, and also allows undo/redo history to be persisted across server restarts.

However, the use of a YStore has led to a number of stability & usability issues in jupyter_collaboration. Specifically:

  • By default, there is no limit to the size of the YStore in .jupyter_ystore.db. Even in single-user scenarios, this file can grow large.

    • In my local Jupyter AI v3 folder, this store grew to 28 MB in just one month of light use 5x a week.
  • The rate at which the DB file grows is proportional to the number of active users per server, as that determines the rate of incoming Yjs updates. This means that if a server has 10 users, the DB file will grow 10x as fast relative to the single-user case.

    • We have directly verified that the YStore can grow to ~100MB given ~5-10 users making updates over a few weeks. This figure seems like an exaggeration, but makes sense when you consider the vast number of actions which can trigger Yjs updates (e.g. typing a single character, running a cell in a notebook).
  • When RTC is enabled, it can take a long time to open a file for the first time after starting a server. This is because when a file is opened for the first time, jupyter_server_ydoc must query a ~10-100MB SQLite file, gather every single Yjs update for that file, then replay all those updates in-memory. This is documented in an upstream issue: [New Issue] When using pycrdt-websocket 0.13.1, file access in jupyterlab can hang jupyter-server/pycrdt-websocket#41

  • Finally, the YStore implementation adds a lot of complexity, which increases the risk of usability bugs. For example, @andrii-i helped identify that notebook and lab can't open in RTC #390 was being caused by the YStore database migration logic not working.

cc @krassowski @afshin @andrii-i @3coins @Zsailer

Proposed Solution

  1. Add a configurable, boolean traitlet which enables/disables the use of YStore.

  2. (contentious) This configurable should default to False, i.e. YStore should not be used by default.

I proposed taking these actions at the JupyterLab Frontends call on 01/15. While we seemed to agree about point 1), we were not able to reach a consensus on point 2).

I argue that the risk & complexity introduced by YStore are not worth the features it enables. Others argue that those features are important and that the real solution is to make YStore more robust.

  • My response: We need to be very realistic about whether we have the engineering resources to quickly refactor & maintain a reliable SQLiteYStore implementation. Several of these bugs have existed for years, and will likely take months/years to fix. Asking users to just wait for more reliability isn't good for users. We should also keep in mind that Jupyter AI v3 depends on Jupyter Collaboration, and that v3.0.0 official will be published in the next 2-4 months.

Additional context

  • Please note: I will likely not have the time to respond to every comment on this issue. I need to focus full-time on Jupyter AI, and I am leaving for a family vacation from 01/24 to 02/02. @3coins and @andrii-i, please engage in this thread if I'm not available.

  • To limit the size of the DB file today, you can configure SQLiteYStore.document_ttl to some integer X. This automatically squashes the Yjs updates for a file if no updates have been received in the last X minutes. However, this feature is undocumented and requires manual intervention. I'm also not sure how confident we can be in the correctness of its implementation given that it's used so infrequently.

@dlqqq dlqqq added the enhancement New feature or request label Jan 16, 2025
@davidbrochart
Copy link
Collaborator

The YStore is here to flush updates to disk, allowing to free up some memory when documents are not changed for some time. Disabling the YStore means keeping all updates in memory, which is not good for a server in terms of scalability.

@afshin
Copy link
Member

afshin commented Jan 17, 2025

As I proposed in yesterday's server call, I think we should do our best to implement a few hardening strategies:

  • We should set bounds for how big a single files updates can be:
    • The number of updates we keep should be bound.
    • The size of large updates (e.g. very large cells) should be taken into account.
  • We should fix whatever edge cases we are missing in the database migration logic from v2 to v3 (as investigated by @andrii-i).
  • We should always fail gracefully. This isn't just a recommendation for jupyter-collaboration but really for all of Jupyter. So if there's any failure in loading and using the database file, I propose we archive/rename/delete that file and create a new one. We cannot impose a broken state on our end users.

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

No branches or pull requests

3 participants