-
Notifications
You must be signed in to change notification settings - Fork 87
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
Depend on jupyter-server only, compatibility with jupyter server >= 2, notebook < 7 #240
Conversation
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.
LGTM! I'm on mobile, assuming circleci failure is unrelated.
We had removed the CircleCI config, but not 'stopped building' in https://app.circleci.com/settings/project/github/jupyterhub/nbgitpuller?return-to=https%3A%2F%2Fapp.circleci.com%2Fpipelines%2Fgithub%2Fjupyterhub%2Fnbgitpuller%3Ffilter%3Dall. I've hit that button now. |
Will this still work in cases where jupyterhub-singleuser is the process being run? |
I have this PR deployed in production (small scale production), and it seems to work—thanks for the fix! |
@akhmerov do you run a jupyterhub, if so, what version, and also if possible: with what commands are jupyter servers started? @yuvipanda i feel a bit inactionable to help nudge this PR to address your review question about jupyterhub-singleuser because i dont understand the connection there may be. Are you looking for an empirical test or is there something specific we should watch out for that can make a setup involving jupyterhubb-singleuser break? |
Thank you for the specific question! So in general I'm super confused about So the test for me is - on an installation that's just running |
I run jupyterhub==2.0.2 and in the container I see I didn't mean to influence the discussion, but FWIW |
I lean towards thinking this is fine and good, but i dont understand it is or isnt with confidence. I think that holds back both me and yuvi from merging atm. One action point to help me dare merge this pr is to provide some discussion if this is a breaking change or not. |
This is breaking, and indeed drops support for the notebook server entirely because auth will fail with Jupyter Server 2 when notebook is the launch application, as seen in this failure:
This is supposed to work (jupyter-server/jupyter_server#1221), but doesn't right now. Even with that fix, it will likely stop working eventually as Jupyter Server moves further away from the classic notebook server setup. If we want to keep support for both applications, we should use a shim to always use the right base handler class, as I do in ipyparallel, here. |
I added the compatibility shim and a test, so this should work correctly on any version or implementation of notebook or jupyter server. It's no longer breaking, purely a compatibility fix. Change:
Fix:
|
FYI Notebook 7 final is now released: https://github.com/jupyter/notebook/releases/tag/v7.0.0 And this PR would help have a version of |
Thanks a lot for getting this set up, @minrk! |
Argh so I'm really not sure how to label this PR. It can be seen as a bug not supporting jupyter_server 2, which was released before nbgitpuller got support for functioning against it, but I also think this kind of in the maintenance / enhancement / new category as well, and I think maybe we should provide a minor release for this PR rather than a patch release? |
I went for enhancement and will open a changelog PR for 1.2.0 |
Looks like notebook is still hanging around and affecting some paths
Closes #235