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

JupyterHub integration #6451

Merged
merged 46 commits into from Jun 9, 2019
Merged

JupyterHub integration #6451

merged 46 commits into from Jun 9, 2019

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 1, 2019

References

Fixes #6428.

cf jupyterhub/jupyterlab-hub#84
cf #6399

Code changes

Absorbs labhubapp and jupyterlab-hub into the main application.

User-facing changes

No longer need to provide any additional config other than setting c.Spawner.default_url = '/lab', and no need to install jupyterlab-hub.

Backwards-incompatible changes

Deprecated SingleUserLabApp.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jun 1, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 1, 2019

@ian-r-rose, do you mind finishing this PR with the connection lost handling?

@blink1073 blink1073 added this to the 1.0 milestone Jun 1, 2019
@blink1073 blink1073 force-pushed the jhub-integration branch 2 times, most recently from d0c8e9f to d3226c1 Jun 3, 2019
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 6, 2019

@ian-r-rose, ping!

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 6, 2019

@blink1073 Sure, I can do that.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 6, 2019

Thanks! ❤️

jupyterlab/labhubapp.py Show resolved Hide resolved
packages/hub-extension/src/index.ts Show resolved Hide resolved
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 7, 2019

I'm currently fixing up some stuff on the Python side -- still not completely running yet.

@@ -204,6 +204,14 @@ def load_jupyter_server_extension(nbapp):
# Must add before the root server handlers to avoid shadowing.
web_app.add_handlers('.*$', handlers)

# If running under JupyterHub, add more metadata.
if hasattr(nbapp, 'hub_prefix'):
Copy link
Member

@ian-r-rose ian-r-rose Jun 7, 2019

This check doesn't appear to be working, at least not in my testing. At the time the server extension is loaded, it doesn't seem like the hub_prefix traitlet has been set.

labhubapp.py seems to be working correctly.

Copy link
Member

@ian-r-rose ian-r-rose Jun 7, 2019

I'm concerned that we won't actually be able to deprecate LabHubApp. Without it, I'm not sure that JupyterHub spawners can easily configure the LabApp specific traitlets (which is a problem I'm having while testing this).

Copy link
Member Author

@blink1073 blink1073 Jun 7, 2019

I'm not sure why you saw this state. I'm seeing it working fine locally...

@blink1073 blink1073 force-pushed the jhub-integration branch from 3c3059c to a33f18d Jun 7, 2019
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 7, 2019

It is working for me in it's current state. Here is the relevant config I'm using:

c.Spawner.default_url = '/lab'
c.Spawner.environment = { 'JUPYTERLAB_DIR': '/home/silvester/workspace/jupyter/lab/dev_mode' }

image

@Zsailer
Copy link
Member

@Zsailer Zsailer commented Jun 8, 2019

@ian-r-rose I may be wrong, but I don't think this problem should happen if traits are added in appropriate config files. Traits in config files should get loaded by the respective classes.

The problem here is that traits passed to the main application being launched by the CLI don't get directed to the classes to which they belong unless those classes are explicitly added to main applications' classes trait.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 8, 2019

@Zsailer My grasp of traitlet loading is definitely pretty shaky, but I was having trouble getting that to work when testing this. My thinking was this: if JupyterHub is using the default SingleUserNotebookApp, then nowhere is a LabApp actually instantiated, and therefore those configurables never get read in from a config file. This is despite the fact that the JupyterLab server extension is activated. Am I missing something?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 8, 2019

Hmm, yeah, configuring an extension itself is really what we want, not an app. If you wanted to configure an app then you should be using that app directly.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 9, 2019

In my ideal world, I think all extensions would be classes, and not use the load_serverextension magic function but something like an import path to the class as the extension load mechanism.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 9, 2019

@ian-r-rose, the error was the fact that we were implicitly relying on the hub extension in our application extension. I added a fallback to the default connectionLost implementation.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 9, 2019

JS failure is unrelated, and the windows one passed, so in she goes. Thanks again @ian-r-rose!

@blink1073 blink1073 merged commit 3d710e0 into jupyterlab:master Jun 9, 2019
7 of 9 checks passed
@Zsailer
Copy link
Member

@Zsailer Zsailer commented Jun 10, 2019

In my ideal world, I think all extensions would be classes, and not use the load_serverextension magic function but something like an import path to the class as the extension load mechanism.

I'm not sure about the history behind the load_jupyter_server_extension, but your suggestion above seems like a good idea. My guess is that for simple server extensions that add a couple and endpoints+handlers, the load_jupyter_server_extension offered a simple plugin mechanism.

More complex extensions would benefit from an import path to class mechanism. jupyter-server/jupyter_server#48 is a compromise of those two solutions. It uses some tricky logic to hijack load_jupyter_server_extension mechanism add configurable extensions. The downside is that we don't get the ability to configure extensions from the jupyter server CLI. Instead, extensions would be configured by their separate config files.

Should we drop the load_jupyter_server_extension mechanism? Or is this still useful for simple extensions?

@Zsailer
Copy link
Member

@Zsailer Zsailer commented Jun 10, 2019

nowhere is a LabApp actually instantiated

@ian-r-rose Ah yes. You're right. I was thinking ahead to a world where hub is launching jupyter_server ServerApps and lab is loaded as a server extension. In that world, LabApp is instantiated and configured from file; however, it (currently) cannot be configured from jupyter_server's CLI.

SylvainCorlay
Copy link
Member

SylvainCorlay commented on 726ce5e Jun 18, 2019

@jasongrout this is a backward incompatible change to @jupyterlab/services.

I have a working update in ipywidgets/jupyterlab-manager but ideally, we should export ReplyContent and ICommInfoReply for clients to @jupyterlab/services.

jasongrout
Copy link
Contributor

jasongrout commented on 726ce5e Jun 18, 2019

You can get the comm reply content with ICommInfoReplyMsg['content'] - is that what you want?

@consideRatio
Copy link
Member

@consideRatio consideRatio commented Jun 18, 2019

Wieeeeeeeeeeeee sooo much excellent work going on ❤️ 🎉! This is awesome!!

@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
@blink1073 blink1073 deleted the jhub-integration branch Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants