jupyter-server / jupyter_ydoc

Jupyter document structures for collaborative editing using Yjs/pycrdt
https://jupyter-ydoc.readthedocs.io
BSD 3-Clause "New" or "Revised" License
27 stars 17 forks source link

Fix joining of `source` when `source` is an array of strings #186

Closed jtpio closed 9 months ago

jtpio commented 9 months ago

This should fix the issue noticed in https://github.com/jupyterlite/jupyterlite/issues/1141#issuecomment-1733613297.

As discussed in https://github.com/jupyterlite/jupyterlite/issues/1141#issuecomment-1733613297, loading notebooks which have cells with their source set to be a list of strings instead of a single string.

Example notebook: https://github.com/jupyterlite/jupyterlite/blob/main/examples/pyodide/ipycanvas.ipynb

Example cell:

{
   "cell_type": "code",
   "execution_count": null,
   "metadata": {},
   "outputs": [],
   "source": [
    "def life_step(x):\n",
    "    \"\"\"Game of life step\"\"\"\n",
    "    nbrs_count = sum(\n",
    "        np.roll(np.roll(x, i, 0), j, 1)\n",
    "        for i in (-1, 0, 1)\n",
    "        for j in (-1, 0, 1)\n",
    "        if (i != 0 or j != 0)\n",
    "    )\n",
    "    return (nbrs_count == 3) | (x & (nbrs_count == 2))"
   ]
  },

Which in JupyterLite renders as follows with extra line breaks:

image

Currently the server component in JupyterLite does not normalize the source to be a string. So it sends the array of strings to the frontend.


This is not an issue in stock JupyterLab because the response from the Jupyter Server already normalizes the source to be a single string:

image

This is likely because Jupyter Server uses nbformat to read the notebook: https://github.com/jupyter-server/jupyter_server/blob/3544deb53902cc02c9aa9d6513b3c30f1113896b/jupyter_server/services/contents/fileio.py#L264-L291

And the nbformat documentation mentions the following in https://nbformat.readthedocs.io/en/latest/format_description.html#cell-types:

On disk, multi-line strings MAY be split into lists of strings. When read with the nbformat Python API, these multi-line strings will always be a single string.

nbformat also joins the string with an empty string: https://github.com/jupyter/nbformat/blob/0a2942c8f77d33b43a327293c7a7596afcf4527d/nbformat/v4/rwbase.py#L38

for cell in nb.cells:
    if "source" in cell and isinstance(cell.source, list):
        cell.source = "".join(cell.source)
davidbrochart commented 9 months ago

Jupyverse sends the cell source as-is, which also leads to extra newlines in JupyterLab. There is a fix in https://github.com/jupyter-server/jupyverse/pull/354, but does it mean this issue is a JupyterLab frontend issue (not particularly an issue in jupyter-ydoc)? Meaning that JupyterLab should render the source correctly if in an array.

jtpio commented 9 months ago

JupyterLab uses the @jupyter/ydoc package, so I would say this fix should be implemented in @jupyter/ydoc as proposed in this PR?

This means https://github.com/jupyter-server/jupyverse/pull/354 would then not be necessary, as the frontend would handle reading an array of strings.

davidbrochart commented 9 months ago

But the issue also appears in non-collaborative mode.

jtpio commented 9 months ago

Because JupyterLab still instantiates a sharedModel: https://github.com/jupyterlab/jupyterlab/blob/bd46c76046671ae375b3ddd207ca441841e57123/packages/docregistry/src/context.ts#L62-L73

And uses shared interfaces exposes by @jupyter/ydoc (search for @jupyter/ydoc in the JupyterLab code base).

davidbrochart commented 9 months ago

I see, thanks. Then I think this PR is good to go. I'll also merge https://github.com/jupyter-server/jupyverse/pull/354 to better align with jupyter-server.

jtpio commented 9 months ago

For reference I was able to verify this does fix the issue by modifying @jupyter/ydoc locally and testing with a local build of JupyterLite (which does not use collaboration):

image

davidbrochart commented 9 months ago

Thanks @jtpio!

jtpio commented 9 months ago

Thanks @davidbrochart for the review!

Would it be possible to get this in a patch release? :pray:

davidbrochart commented 9 months ago

Done: https://pypi.org/project/jupyter-ydoc/1.1.1

jtpio commented 9 months ago

Thanks!