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 support for opening a document with a different factory #6315

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Mar 18, 2022

First steps towards jupyterlab/retrolab#290.

For example when opening a JSON file with different factories:

open-factory-edit.mp4

This is also useful so folks can open notebooks with the text editor too:

open-factory-notebook.mp4

TODO

  • Basic support for custom factories when opening a document
  • Should this work for arbitrary widgets, like the settings editor? Decide whether to do this in this PR or as follow-up -> deferring to another PR / issue, see this comment for a proof of concept: Add support for opening a document with a different factory #6315 (comment)
  • Should the ?factory= URL parameter be removed from the URL after the document is opened? -> keeping it when it is not default, so the page can be reloaded and the document renders the same

@jtpio jtpio added this to the 7.0 milestone Mar 18, 2022
@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch jtpio/notebook/factories

@jtpio jtpio changed the title Add support for opening a document with a factory Add support for opening a document with a different factory Mar 18, 2022
@jtpio
Copy link
Member Author

jtpio commented Mar 18, 2022

Should the ?factory= URL parameter be removed from the URL after the document is opened?

Keeping it could be useful, so it can be copy pasted and sent to other people (or bookmarked). But some folks used to the classic notebook might wonder what this actually means.

@jtpio
Copy link
Member Author

jtpio commented Mar 18, 2022

Should this work for arbitrary widgets, like the settings editor?

Probably this one could have special treatment.

Right now the NotebookShell only allows one widget to be added to the main area:

if (this._main.widgets.length > 0) {
// do not add the widget if there is already one
return;
}

An alternative could be to add it to the tree / dashboard page:

image

@jtpio
Copy link
Member Author

jtpio commented Mar 18, 2022

An alternative could be to add it to the tree / dashboard page:

Some proof of concept to explore this:

tree-setting-editor.mp4

Posting here for visibility, but should probably be explored more in another PR.

@fcollonval
Copy link
Collaborator

Thanks a lot for starting @jtpio.

I got a call yesterday with @parmentelat about using Jupytext in notebook v7 (something we cannot overlook). Starting from v1.13.2, Jupytext uses the document factory to display files with the notebook editor. So we definitely should check this PR works with Jupytext.

cc @mwouts

@jtpio
Copy link
Member Author

jtpio commented Mar 19, 2022

Thanks @fcollonval for the heads up. I think it should work with the Jupytext Notebook factory. Maybe we'll need to make sure it properly opens with the /notebook URL.

I installed jupytext with pip install jupytext locally and the server and lab extensions seem to be properly installed.

$ jupyter server extension list
Config dir: PREFIX/etc/jupyter
    jupyterlab enabled
    - Validating jupyterlab...
      jupyterlab 4.0.0a22 OK
    jupytext enabled
    - Validating jupytext...
      jupytext 1.13.7 OK
    notebook_shim enabled
    - Validating notebook_shim...
      notebook_shim  OK
$ jupyter labextension list
JupyterLab v4.0.0a22
PREFIX/share/jupyter/labextensions
        jupyterlab-jupytext v1.3.8 enabled  X (python, jupytext)
        @jupyter-notebook/lab-extension v7.0.0-alpha.1 enabled OK

The lab extension loads fine. However the jupytext server extension seems to be failing on startup: with

[W 2022-03-19 17:15:02.569 ServerApp] jupytext | extension failed loading with message: name 'build_jupytext_contents_manager_class' is not defined

@jtpio
Copy link
Member Author

jtpio commented Mar 19, 2022

Ah looks like it tries to import LargeFileManager from notebook.services.contents.largefilemanager here:

https://github.com/mwouts/jupytext/blob/d25c0c640ca0b2c1fb0e15b99be81bb82d2fe5ab/jupytext/contentsmanager.py#L573-L581

Which could be related since these are not available in Notebook v7... (but could be shimmed?)

Does jupytext work with Jupyter Server / JupyterLab only? Or does it expect the classic notebook server (via the notebook package) to be available too?

@jtpio
Copy link
Member Author

jtpio commented Mar 20, 2022

Ah looks like it tries to import LargeFileManager from notebook.services.contents.largefilemanager here:

This looks good when running with the fix proposed in mwouts/jupytext#933:

jupytext-factory.mp4

Just need to open it under /notebooks to match the behavior of the classic notebook v6.

@fcollonval
Copy link
Collaborator

fcollonval commented Mar 20, 2022

Thanks a lot for testing it with Jupytext.

Just need to open it under /notebooks to match the behavior of the classic notebook v6.

How do you plan to support that case? I think we will be able to supported it out of the box by applying jupyterlab/jupyterlab#11540 in Jupytext.

@jtpio
Copy link
Member Author

jtpio commented Mar 20, 2022

I think we will be able to supported it out of the box by applying jupyterlab/jupyterlab#11540 in Jupytext.

Maybe only the Notebook factory should open in /notebooks then? Looks like this would indeed work out of the box with this PR?

@jtpio jtpio marked this pull request as ready for review March 21, 2022 09:55
@jtpio
Copy link
Member Author

jtpio commented Mar 21, 2022

Documents now open with their default factory, for example when double-clicking on the item in the file browser.

When opened with a different factory, ?factory= is kept in the URL so the browser tab can be reloaded and the document renders the same:

default-factory.mp4

This also means that some files like JSON now open with the JSON viewer instead of the text editor by default. This differs from the behavior of the classic notebook and could maybe be customized via the settings if needed. Opening with the default JSON viewer by default is however useful because it's coherent with the behavior in JupyterLab.

@jtpio
Copy link
Member Author

jtpio commented Mar 21, 2022

cc @fcollonval if you would like to have a look.

Happy to get this in and iterate further. We can also make a new alpha release if that can ease development of third-party extensions like jupytext.

Copy link
Collaborator

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this one.

@parmentelat
Copy link
Contributor

indeed @jtpio many thanks !

@jtpio jtpio mentioned this pull request Mar 21, 2022
5 tasks
@jtpio
Copy link
Member Author

jtpio commented Mar 21, 2022

Thanks @fcollonval and @parmentelat!

Let's get this in then, and I'll start a new release right after.

@jtpio jtpio merged commit 9be03f5 into jupyter:main Mar 21, 2022
@jtpio jtpio deleted the factories branch March 21, 2022 15:05
@fcollonval
Copy link
Collaborator

Maybe only the Notebook factory should open in /notebooks then?

Looking into this briefly, the trouble for jupytext are actually for file types already defined in core (like .py or .md). I was thinking to use the Router to deal with path like /notebooks/not/a-ipynb-file.ext URL to redirect to the /edit/not/a-ipynb-file.ext?factory=Jupytext%20Notebook if the extension is supported by Jupytext. What do you think @jtpio ?

@jtpio
Copy link
Member Author

jtpio commented Mar 21, 2022

Thanks @fcollonval let's track this in #6324.

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

Successfully merging this pull request may close these issues.

None yet

3 participants