Allow YRoom to be restarted and allow multi-room kernels
#117
Conversation
dlqqq
force-pushed
the
restart-inactive-rooms
branch
from
9f0b13e to
aff427f
Compare
dlqqq
changed the title
|
@ellisonbg I've added two new methods to
The observers added by this method are automatically migrated when the YDoc is reset. The old API for adding an observer to the JupyterYDoc (shown below) should not be used anyways, because the default implementation only allows exactly 1 observer per YDoc. # old API that should be avoided
jupyter_ydoc = await room.get_jupyter_ydoc()
jupyter_ydoc.observe(...)
# ^ this call actually removes any observers previously added via `jupyter_ydoc.observe()`The only major change for consumers is that consumers will have to use the top-level observer, and not add an observer to a shared type within the YDoc. This can always be worked around because the top-level observer fires every time a shared type within itself is updated. Here is an example of how a consumer can migrate their observer to use the new API: def observer(event: pycrdt.ArrayEvent):
...
# old API that should be avoided
ychat = await yroom.get_jupyter_ydoc()
ychat.messages.observe(observer)
# new API equivalent
yroom.observe_jupyter_ydoc(
lambda updated_key, event: observer(event) if updated_key == "messages" else None
)Since JupyterYDoc is just a custom API wrapper around the YDoc, we don't need a corresponding set of methods for |
|
We probably need more time to discuss/explore how to handle stale references to YDoc's in the server. I am a bit surprised that Ydocs only allow a single observer. How can that be? I have already seen a demo of multiple AI personas watching the same doc with their own observers. |
|
I also don't think there is any way we can tell people to not use the existing YDoc APIs (such as observers). |
dlqqq
changed the title
YRoom to be restarted and allow multi-room kernels
|
Discussed this with @ellisonbg at standup. We should not auto-restart rooms until #120 is addressed. I've disabled the We agreed that this PR should still be merged since it brings API improvements, fixes #111, and lays out the foundation for fixing #60. |
| _room_ids: dict[str, str] | ||
| """ | ||
| Dictionary of room IDs, keyed by session ID. | ||
| """ | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self._room_ids = {} |
There was a problem hiding this comment.
Nit: A more traitlets-y way to write this logic 🙂
| _room_ids: dict[str, str] | |
| """ | |
| Dictionary of room IDs, keyed by session ID. | |
| """ | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| self._room_ids = {} | |
| # Dictionary of Room IDs, keyed by session ID. | |
| _room_ids = Dict({}) |
|
@Zsailer Thank you for giving this a look Zach! Brian had already asked that I migrate all of these classes to use Merging now so I can start my next PR without merge conflicts. 👍 |
4 participants
Description
This PR is a re-implementation of #114 that restarts empty rooms instead of deleting them. This frees the memory occupied by its YDoc history while preserving the references to
YRoom.YRooms #60UPDATE: the
_watch_rooms()background task has been disabled. This PR is now just refactors theYRoomandYRoomFileAPIto allow them to be restarted, while also adding support for multiple notebooks sharing a kernel. We should not enable the_watch_rooms()task until #120 is addressed.Technical details
YRoomManager
The
YRoomManagernow has a_watch_rooms()background task that runs every 10 seconds, checking each room. This task has been disabled due to #120.When the room is restarted depends on whether it provides a notebook:
For notebook rooms: the room is restarted when there are no connected clients AND when the kernel is either 'idle' or 'dead' for >10 seconds (2 iterations of inactivity).
For all other rooms: the room is restarted when there are no connected clients for >10 seconds.
Rooms are also not restarted if they have no updates in the YDoc history. Therefore, empty rooms will only be restarted once, not every 10 seconds they remain inactive.
YRoom & YRoomFileAPI
The main difference is that these classes is that both provide
stop()andrestart()methods that are fully synchronous.YRoom.stop()now saves the final content of the YDoc in a background task through theYRoomFileAPI.YRoomto use its methods concurrently,YRoomFileAPInow uses a_contents_lockto prevent overlapping ContentsManager calls to the same file.Guidance on testing this PR
Non-notebook rooms
Open a text file, make a change, then close the text file. Watch the server logs, and you should see statements similar to the ones below printed after 10-20 seconds:
This asserts that non-notebook rooms are restarted after they have no connected clients for >10 seconds.
Then, you can try opening a text file, not make changes, then close it. You should see that the room is not restarted. This asserts that the room is restarted only if an update was made to its YDoc history, i.e. only if there is memory to be freed.
Notebook rooms
Create a new notebook with this code cell:
Run this cell and close the notebook. Watch the logs and verify that it takes more than 20 seconds before the room is restarted. After the room is restarted (~40 seconds), open the notebook, and you should see that
"hello"is correctly printed in the output. This asserts that the room manager does not restart a notebook room while its kernel is still running (i.e. its execution state is not "idle" or "dead").You can use the kernel again after re-opening the notebook to verify that everything still works as expected after the room is restarted.
Follow-up issues
YRoom#119