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

Add room storage for Django Channels Consumers #25

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

cacosandon
Copy link

This PR introduces a shared storage for consumers within Django Channels.

Initially, each YjsConsumer had its unique YDoc, updating only from its client. This approach caused issues, such as consumers having different YDoc versions that made difficult to update to a persistent storage.

Now, by inheriting from BaseYRoomStorage, a unified storage can be utilized across all consumers and server-wide (e.g., in a Django Celery Job).

The storage's function, detailed in the documentation, is to manage document changes—fetching, updating, and saving them to a single source. An implementation example features Redis for ephemeral and Postgres for permanent storage.

Each user can define their own storage. Maybe using Django Cache, custom Redis or no storage at all (and just use it to broadcast updates from their connected clients). We just define the interface.

Copy link

welcome bot commented Mar 29, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@davidbrochart
Copy link
Collaborator

Thanks @cacosandon for the PR.
At first sight, I'm wondering if you're not reimplemention the ystores?

@cacosandon
Copy link
Author

cacosandon commented Mar 30, 2024

Thanks @cacosandon for the PR. At first sight, I'm wondering if you're not reimplemention the ystores?

Hey @davidbrochart! Sorry for the delay.

The core distinction between YStore (and YRoom) and Django Channels lies in their operational contexts. As I see, YStore is designed for single-server environments, orchestrating multiple rooms with explicit start and stop controls. This centralized model allows for unified management of YDoc instances across rooms.

On the other hand, Django Channels uses a decentralized model, where each consumer operates in an independent execution thread. Our approach offers an "ephemeral" storage solution, keeping everyone in sync without manually managing each document's store lifecycle.

In summary, both systems aim to synchronize and persist document states. However, the decentralized nature of Django Channels requires a YDoc to be synced outside the server. We can adapt the current setup to use YRoom and YStore, but we'll likely use only functions for snapshots, saving, updating, etc., not the lifecycle methods these classes were intended for. Also, because there's no centralized document, we need this "ephemeral" storage, which essentially holds the shared YDoc (that could be a global variable, a file, or Redis, as we've shown in an example) that differs with the current implementation, which initiates an instance of the store when the server start.

Let me know what u think!

@cacosandon
Copy link
Author

Hey @davidbrochart, have you taken a look at this? 🙏

Copy link
Collaborator

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @cacosandon.
I'm not sure about these changes, I still think that YRoomStorage is similar to YStore.

@@ -0,0 +1,3 @@
from .yjs_consumer import YjsConsumer as YjsConsumer
from .yroom_storage import BaseYRoomStorage as BaseYRoomStorage
from .yroom_storage import RedisYRoomStorage as RedisYRoomStorage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that redis is a required dependency when using Django channels?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, right. I've removed RedisYRoomStorage from this init file and separate them in files so when you import the base class you are not requiring redis. Maybe we should do the same with sqlite-anyio on YStore, which is a dependency for the whole project.

pycrdt_websocket/django_channels/yroom_storage.py Outdated Show resolved Hide resolved
pycrdt_websocket/django_channels/yroom_storage.py Outdated Show resolved Hide resolved
@cacosandon cacosandon force-pushed the django-channels/add-room-storage branch from f4c2b06 to f7c6f26 Compare April 6, 2024 19:30
@cacosandon
Copy link
Author

cacosandon commented Apr 6, 2024

Sorry for the delay @cacosandon. I'm not sure about these changes, I still think that YRoomStorage is similar to YStore.

Thank you, @davidbrochart, for the comments!

I think the primary distinction lies in our need for a shared YDoc that isn't directly within an instantiated class, as the repository's main server does. This shared doc is facilitated through YRoomStorage.

While they are very similar—implementing the same methods for applying updates and writing—what they lack is an "ephemeral" storage to manage each of the updates from clients (or consumers in Django Channels).

Additionally, we don't require the property started or __aenter__, __aexit__, start, and stop methods (because Django Channels it's a decentralized model), but we do need others to synchronize the updates before writing to the database (if necessary).

Thus, while we could use YStore, duplicating it in this case to manage each separately seems preferable. But I am still open to that possibility.

Let me know what you think, and thanks again :)

@cacosandon cacosandon requested a review from davidbrochart April 6, 2024 20:11
@davidbrochart
Copy link
Collaborator

While they are very similar—implementing the same methods for applying updates and writing—what they lack is an "ephemeral" storage to manage each of the updates from clients (or consumers in Django Channels).

There could be a new in-memory implementation of a YStore.

Additionally, we don't require the property started or __aenter__, __aexit__, start, and stop methods (because Django Channels it's a decentralized model), but we do need others to synchronize the updates before writing to the database (if necessary).

Usually services have to do some kind of initialization and teardown, but if you don't need that then nothing prevents you from doing nothing in these methods.

Thus, while we could use YStore, duplicating it in this case to manage each separately seems preferable.

But that's also a maintenance effort that I'd rather not do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants