-
Notifications
You must be signed in to change notification settings - Fork 304
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
Conversation
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. |
@Zsailer A few feedback.
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. |
Thanks @echarles! Diving into these issues now... |
@echarles This should be fixed now. |
@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 This makes me think that if I will now look on this works for jlab. |
@Zsailer @blink1073 All good with this server branch for JuypyterLab. JupyterLab works fine with 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! |
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. |
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) :
@echarles, for JupyterLab, 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. |
I'm looking into this now. |
Have you tested all of that with traitlets master ? I'm looking into making a beta soon. |
@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 |
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. |
Thanks, @Carreau. I'll do a deeper dive into this issue with traitlets master early next week. |
@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 |
925ca4b
to
9353e2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
tests/extension/test_manager.py
Outdated
from .mockextensions import _jupyter_server_extension_paths | ||
|
||
# Testing the first path (which is an extension app). | ||
metadata_list = _jupyter_server_extension_paths() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This PR is mostly ready to go. I've got a little more work to do on the |
…ttributes instead of instance attributes
0deba0a
to
3b09860
Compare
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
🎉🎉🎉 Thx a lot @Zsailer for this and to @blink1073 for the review/merge. I am will align jupyterlab_server on this. |
Add new extension manager API
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.