-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Some fixes for Jupyter Notebook 7 #1944
Some fixes for Jupyter Notebook 7 #1944
Conversation
@@ -38,7 +38,8 @@ WORKDIR /tmp | |||
RUN mamba install --yes \ | |||
'notebook' \ | |||
'jupyterhub' \ | |||
'jupyterlab' && \ | |||
'jupyterlab' \ | |||
'nbclassic' && \ |
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.
It seems that we need to install this explicitly, and nbclassic
is kept to make migration easier. So, our images should also support nbclassic
.
https://jupyter-notebook.readthedocs.io/en/stable/migrating/multiple-interfaces.html#nbclassic-and-notebook-7
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.
That's right. The classic notebook UI is then available under the /nbclassic
endpoint.
base-notebook/start-notebook.sh
Outdated
@@ -19,4 +19,4 @@ if [[ "${RESTARTABLE}" == "yes" ]]; then | |||
fi | |||
|
|||
# shellcheck disable=SC1091,SC2086 | |||
exec /usr/local/bin/start.sh ${wrapper} jupyter ${DOCKER_STACKS_JUPYTER_CMD} ${NOTEBOOK_ARGS} "$@" | |||
exec /usr/local/bin/start.sh ${wrapper} jupyter ${DOCKER_STACKS_JUPYTER_CMD} --port ${JUPYTER_PORT} ${NOTEBOOK_ARGS} "$@" |
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.
JUPYTER_PORT
env variable is no longer considered as a port for jupyter applications, so we pass this explicitly - I think the easiest way to do this is to add this line to both start-notebook.sh
and start.sh
.
We, on the other hand, can and should keep the variable name JUPYTER_PORT
for users of our docker image.
This way we won't break them (unless they call start.sh
manually, but I suppose it's a rare case).
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.
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.
Since it's part of Jupyter Server now: https://github.com/jupyter-server/jupyter_server/blob/c1d689e05169ba47e630ccaa39cebb4f847b3562/jupyter_server/serverapp.py#L950
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.
Thanks, I will try to revert this part.
@jtpio may I ask you to take a look here - I think you know quite well if this is a reasonable change after Jupyter Notebook 7 release. |
@@ -64,7 +64,7 @@ RUN mamba install --yes \ | |||
# Install facets which does not have a pip or conda package at the moment | |||
WORKDIR /tmp | |||
RUN git clone https://github.com/PAIR-code/facets.git && \ | |||
jupyter nbextension install facets/facets-dist/ --sys-prefix && \ | |||
jupyter nbclassic-extension install facets/facets-dist/ --sys-prefix && \ |
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.
It seems that the command name has changed.
https://discourse.jupyter.org/t/notebookv7-nbclassic-how-do-you-enable-disable-classic-extensions-from-the-command-line/16344
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.
For reference there is also a note in the docs: https://nbclassic.readthedocs.io/en/latest/nbclassic_dev_faq.html#noteworthy-updates-in-nbclassic
Thanks @mathbunnyru for looking into this! I left a couple of comments, looking good otherwise 👍 |
@mathbunnyru @jtpio Since this pull request got merged (b8d617d), zero-to-jupyterhub-k8s always launches the singleuser jupyter environment in the classic notebook tree view, instead of the JupyterLab mode. I tried many different configuration options to force it otherwise, but I did not manage to change it. If I go back to the previous docker-stacks image (141d72d, there are only documentation related changes between the merged pull request and that version), it works as expected and I got the |
Thanks @kondi for reporting. I wonder if this might be related to jupyter/notebook#6954. |
@kondi @jtpio I also think there is a bug in the JupyterHub which won't allow to choose Jupyter Notebook properly by setting |
* Some fixes for Jupyter Notebook 7 * Fix jupyter nbextension * Remove explicit port setting
* Some fixes for Jupyter Notebook 7 * Fix jupyter nbextension * Remove explicit port setting
Describe your changes
Issue ticket if applicable
Checklist (especially for first-time contributors)