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 new extension manager API #248

Merged
merged 30 commits into from
Jul 31, 2020

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Jun 25, 2020

This PR adds a new API for discovering, linking, and loading Jupyter Server extensions. It introduces an ExtensionManager for interfacing with server extensions.

This should be backward compatible with all current server extensions while remaining flexible enough for future development of server extensions and the ExtensionApp.

This addresses issues #207, #222, #224, and #243.

@echarles, I'm curious to get your thoughts on this PR.

@echarles
Copy link
Member

Awesome @Zsailer. One PR for some many critical issues, you rock!! 👍 💯 🥇

I will prolly not have time before today weekly meeting to do that, but I will try this branch on with the jlab server extensions.

Have you tried the example having another name (eg. foo) and run jupyter --generate-config (I would hope it will create a jupyter_foo_config.py file) ?

jupyter_server/serverapp.py Outdated Show resolved Hide resolved
@echarles
Copy link
Member

echarles commented Jun 25, 2020

@Zsailer A few feedback.

  • During today meeting, I said that changing the extension name from simple_ext_1 to foo was working fin, but actually, I had simple_ext_1 enabled via json. I removed the json, and it does not work...[W 2020-06-25 23:01:06.752 ServerApp] The module 'foo' could not be found. Are you sure the extension is installed?
  • I have run jlab jupyter_server branches on this branch and no regression. It works like before with the name='jupyterlab'
  • I have changed to name from 'jupyterlab' to 'lab', and same error as for the examples: [W 2020-06-25 23:04:19.821 ServerApp] The module 'lab' could not be found. Are you sure the extension is installed?

I think you catch the exception and gives a warning which is ok, but the exception is still there. The generate config works fine, but when you start the server, the extension is not enabled if the extension name is different from the package name.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 26, 2020

Thanks @echarles! Diving into these issues now...

@Zsailer
Copy link
Member Author

Zsailer commented Jun 26, 2020

@echarles This should be fixed now.

@echarles
Copy link
Member

echarles commented Jun 27, 2020

@Zsailer Thx again for supporting and implementing this effort. I sometimes feel bad reporting those issues and not be able to find time to contribute on the code base...

I have changed the example name to foo and invoked jupyter server --ServerApp.jpserver_extensions="{'simple_ext1': True}" - The UI is now available on http://localhost:8888/foo/default which is what we want I think.

This makes me think that if simple_ext1 module has multiple extensions (foo, bar...) they will all be enabled or disabled at the same time. But maybe this can considered as a feature because we want all extension of a module to live or die together. Curious to read your thought on that.

I will now look on this works for jlab.

@echarles
Copy link
Member

echarles commented Jun 27, 2020

@Zsailer @blink1073 All good with this server branch for JuypyterLab.

JupyterLab works fine with extension_name='lab' so that jupyter --generate-config creates a jupyter_lab_config.py file.

I will do a sanity pass on the 2 jlab branches and will give the green light for deep reviews and comments at the next jlab weekly meeting. I hope we will be able to converge to a merge.

I will also take time before the next jserver meeting to do a pass on this PR. When comments will be addressed we could plan a server and nbclassic releases so that the jlab branches can use those releases instead of snapshots.

Thx all!

@echarles
Copy link
Member

I have been looking at existing jlab extensions with server component. As the jlab branch is aligned with master (3.0.0.-alpha.0), I have not found any jlab extension working on that. So I have tried one of my stuff and so far it is not working.

That extension is working fine with jlab on classic notebook, but not on jlab on jserver-ext.

https://github.com/datalayer/datalayer/blob/ccee92ff88991c4b77f41f78efa27d113ee1ea47/src/jupyter/lab/twitter/jupyter_twitter/__init__.py

@Zsailer
Copy link
Member Author

Zsailer commented Jun 29, 2020

This makes me think that if simple_ext1 module has multiple extensions (foo, bar...) they will all be enabled or disabled at the same time. But maybe this can considered as a feature because we want all extension of a module to live or die together. Curious to read your thought on that.

You're right. We should standardize our language around this, too. Here's the language I've been using in the code-base (we should discuss if these definitions are clear, too) :

  • Extension package: the parent Python package with an __init__.py that defines a _jupyter_server_extension_paths function. This function returns a list of metadata describing extensions points in that package (i.e. submodules that define a _load_jupyter_server_extension).
  • Extension points: submodules within the package that define a _load_jupyter_server_extensions function and include the logic that extends the server.

@echarles, for JupyterLab, jupyterlab is an extension package and the LabApp, named lab, is an/the extension point in that package.

Enabling/disabling happens at the extension package level. All extensions points in that package are appended to the server when the extension package is enabled.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 29, 2020

That extension is working fine with jlab on classic notebook, but not on jlab on jserver-ext.

I'm looking into this now.

@Carreau
Copy link
Contributor

Carreau commented Jul 6, 2020

Have you tested all of that with traitlets master ? I'm looking into making a beta soon.

@echarles
Copy link
Member

echarles commented Jul 7, 2020

@Carreau pytest run fines on this branch with latest released traitlets.

With traitlets master it fails with

    def test_initialize(mock_extension, template_dir):
        # Check that settings and handlers were added to the mock extension.
        assert isinstance(mock_extension.serverapp, ServerApp)
>       assert len(mock_extension.handlers) > 0
E       assert 0 > 0
E        +  where 0 = len([])
E        +    where [] = <tests.extension.mockextensions.app.MockExtensionApp object at 0x10da1cbe0>.handlers

tests/extension/test_app.py:31: AssertionError

@Carreau
Copy link
Contributor

Carreau commented Jul 7, 2020

Ok, I had a quick look, but it seem relatively complex when/what is supposed to initialize those handler. If you narrow that down a bit more to which traitlets behavior created that I can help you to either fix it here or land a change in traitlets.

@Zsailer
Copy link
Member Author

Zsailer commented Jul 9, 2020

Thanks, @Carreau. I'll do a deeper dive into this issue with traitlets master early next week.

@echarles
Copy link
Member

@Zsailer Ping me when you feel it is ok to test JupyterLab with this branch combined with https://github.com/Zsailer/nbclassic/pull/12. Thx

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Looking good!

jupyter_server/extension/utils.py Show resolved Hide resolved
from .mockextensions import _jupyter_server_extension_paths

# Testing the first path (which is an extension app).
metadata_list = _jupyter_server_extension_paths()
Copy link
Contributor

Choose a reason for hiding this comment

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

Extension points? Or did you decide against the rename?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, you're totally right. I've updated with the new name. I still catch the old function name but a throw a debug message that this might change in the future.

@Zsailer
Copy link
Member Author

Zsailer commented Jul 27, 2020

This PR is mostly ready to go. I've got a little more work to do on the jupyter server extension CLI, then I'll ping here for deep review.

@Zsailer
Copy link
Member Author

Zsailer commented Jul 29, 2020

Nearly finished—I need to make some small changes in the enabling/disabling application class. I'll have these done by our Jupyter Server meeting on Thursday. Then, this should be ready to merge.

@Zsailer
Copy link
Member Author

Zsailer commented Jul 31, 2020

Ok! This is ready to go!

I'm going to update the docs in a separate PR. I'd like to focus the review on the code itself.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

🎉

@blink1073 blink1073 merged commit 703a711 into jupyter-server:master Jul 31, 2020
@echarles
Copy link
Member

echarles commented Aug 1, 2020

🎉🎉🎉 Thx a lot @Zsailer for this and to @blink1073 for the review/merge. I am will align jupyterlab_server on this.

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.

5 participants