[WIP] Fix issues when using RTCore Yroom #38
Conversation
| self._ydoc = pycrdt.Doc() | ||
| self._awareness = pycrdt.Awareness(ydoc=self._ydoc) | ||
| _, file_type, _ = self.room_id.split(":") | ||
| file_type, _, _ = self.room_id.split(":") |
There was a problem hiding this comment.
The room ID takes the format {file_format}:{file_type}:{file_id}, so the existing code here is correct. This confusion is my fault; the docstring on the room_id attribute is incorrect.
Here's the reference code in jupyter_collaboration: https://github.com/jupyterlab/jupyter-collaboration/blob/b9ccca66f30fbc4d5cd10eeec32ab5ca83f25f91/packages/docprovider/src/yprovider.ts#L107
There was a problem hiding this comment.
To clarify, the docstring in yroom.py that reads:
"room_id := "{file_type}:{file_format}:{file_id}"
should be rewritten as:
"room_id := "{file_format}:{file_type}:{file_id}"
| # formats & file types. | ||
| room_id: str | ||
| file_format: Literal["text", "base64"] | ||
| file_format: Literal["text", "base64", "json"] |
There was a problem hiding this comment.
The list of available file formats is determined by the ContentsManager. I don't think json is an accepted file format, based on the docstring here: https://github.com/jupyter-server/jupyter_server/blob/4ee6e1ddc058f87b2c5699cd6b9caf780a013044/jupyter_server/services/contents/filemanager.py#L421-L426
@Zsailer can you help confirm?
There was a problem hiding this comment.
Wait, Naresh's changes here look correct. Here is the room ID I get when I try to open a notebook:
Initializing room 'json:notebook:c3367595-c0ad-4946-a89b-abe0419cc4fa'.
Based on this, it seems format can also be 'json'.
| try: | ||
| assert self.jupyter_ydoc | ||
| path = self.get_path() | ||
| self.log.info(f"Saving content for room ID '{self.room_id}'. {path}. {self.file_format} {self.file_type}") |
There was a problem hiding this comment.
This will log every single time a save is processed. I recommend removing this / changing it to a debug log to prevent a flood of log messages by default.
| def build_notebook_room_id(self, file_id, file_type) -> str: | ||
| # TOOD: extract/how to construct file format? | ||
| return f"{file_type}:notebook:{file_id}" | ||
|
|
There was a problem hiding this comment.
The notebook itself is the file type. The first component should be the file format, which should be json for notebooks. It seems the variables should be renamed accordingly:
| def build_notebook_room_id(self, file_id, file_type) -> str: | |
| # TOOD: extract/how to construct file format? | |
| return f"{file_type}:notebook:{file_id}" | |
| def build_notebook_room_id(self, file_id, file_format: str | None = 'json') -> str: | |
| # TOOD: extract/how to construct file format? | |
| return f"{file_format}:notebook:{file_id}" | |
2 participants
I have been trying to fix issues that I was encountering when I am integrating kernel manager with new YRoom. These are not correct fixes but shared a draft to discuss