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

Jupyter Notebook deprecation -> default to JupyterLab #1575

Merged
merged 10 commits into from
Jan 20, 2022

Conversation

romainx
Copy link
Collaborator

@romainx romainx commented Jan 19, 2022

Hello,

This PR intends to close #1217.

The idea is to

  • switch to JupyterLab by default i.e. running the jupyter lab command
  • permit to run different commands in order to switch back for example to the classic Jupyter Notebook.

Instead of defining a closed option, I think it's more flexible to define a new option JUPYTER_CMD that can be used to launch different jupyter commands.

Command Backend Frontend
lab (def) Jupyter Server JupyterLab
notebook Jupyter Notebook Jupyter Notebook
nbclassic Jupyter Server Jupyter Notebook
server Jupyter Server None?
retro* Jupyter Server RetroLab

*Not installed at this time, but it could be the case in the future or it could be customized by end users.
Thanks 🙏 @krassowski, your answer on SO helped me a lot to better understand the Jupyter landscape.

For example to switching back to classic notebook backend and frontend.

docker run -it --rm -p 8888:8888 -e JUPYTER_CMD=notebook jupyter/base-notebook
# Executing the command: jupyter notebook

At this time I've not checked if the value is in a list, if not it will be managed by the jupyter command.

docker run -it --rm -p 8888:8888 -e JUPYTER_CMD=foo jupyter/base-notebook
# Jupyter command `jupyter-foo` not found.

Warning and deprecation

I've left a warning that is intended to tell that JUPYTER_ENABLE_LAB is no more used and has no effect.
I'm not sure we have to manage here Jupyter Notebook deprecation since we do not launch it anymore by default.
If the user decides to run it intentionally, it's not necessary to raise a warning it should be done by Jupyter Notebook.

Todo

  • Decide if we want to use this solution -> 👍
  • Decide if we want to perform additional control to check if JUPYTER_CMD holds a proper value -> 👎
  • Decide if we want to keep a deprecation notice about the JUPYTER_ENABLE_LAB flag -> 👍 but it has to be changed
  • Update or remove the deprecation notice
  • Update the documentation
  • Improve test coverage

@mathbunnyru and @consideRatio I would like to have your opinion. Once decided I will take care of the remaining tasks.

Many thanks 😄

Introduce a new `JUPYTER_CMD` option
@romainx romainx marked this pull request as draft January 19, 2022 11:24
@manics
Copy link
Contributor

manics commented Jan 19, 2022

It's there any sort of standard variable across the Jupyter ecosystem for defining the Jupyter app/command?

This has kind of come up in JupyterHub before, so it'd be nice if everyone in Jupyter could use a common variable!

@romainx
Copy link
Collaborator Author

romainx commented Jan 19, 2022

@manics Agree, however I don't know. I've only seen JUPYTERHUB_SINGLEUSER_APP from here but I don't think it can be reused here. If someone has the answer I will be happy to use it instead.

@benz0li
Copy link
Contributor

benz0li commented Jan 19, 2022

What about jupyterhub-sigleuser in

. /usr/local/bin/start.sh jupyterhub-singleuser ${NOTEBOOK_ARGS} "$@"

Should this be changed to jupyter-labhub?

@romainx
Copy link
Collaborator Author

romainx commented Jan 19, 2022

@benz0li good point! I will have a look 👀.

Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

I do like this solution - it's quite clear what's gonna be launched.

One thing to add about this solution - we should probably parametrize more tests based on what we launch, it will make our test suite work in more situations.

base-notebook/test/test_start_container.py Outdated Show resolved Hide resolved
@benz0li
Copy link
Contributor

benz0li commented Jan 19, 2022

Should this be changed to jupyter-labhub?

Does not seem to be necessary:


JupyterHub < v2.0 (JupyterLab ≥ v3.0)

Opens Jupyter Notebook by default.
ℹ️ Setting c.Spawner.default_url = '/lab' in jupyterhub_config.py will open JupyterLab by default [when using the NotebookApp backend].

--

By default, the single-user notebook server uses the (old) NotebookApp from the notebook package.

Configuring user environments — JupyterHub 1.4.2 documentation > Switching to Jupyter Server

ℹ️ Setting c.Spawner.environment = { 'JUPYTERHUB_SINGLEUSER_APP': 'jupyter_server.serverapp.ServerApp' } in jupyterhub_config.py will use Jupyter Server’s ServerApp backend.
👉 Setting c.Spawner.default_url = '/lab' or c.Spawner.default_url = '/tree' in jupyterhub_config.py is required.


JupyterHub ≥ v2.0 (JupyterLab ≥ v3.0)

By default the single-user server launches JupyterLab, which is based on Jupyter Server.

Configuring user environments — JupyterHub 2.0.0 documentation > Switching back to classic notebook

ℹ️ Setting c.Spawner.default_url in jupyterhub_config.py has no effect.

--

The ServerApp is the default backend when running JupyterHub ≥ 2.0.

ℹ️ Setting c.Spawner.environment = { 'JUPYTERHUB_SINGLEUSER_APP': 'notebook.notebookapp.NotebookApp' } in jupyterhub_config.py will use the (old) NotebookApp backend.
👉 Opens Jupyter Notebook by default. Setting c.Spawner.default_url = '/lab' in jupyterhub_config.py will open JupyterLab by default.

@romainx
Copy link
Collaborator Author

romainx commented Jan 19, 2022

@benz0li thank you and I confirm about jupyterhub-singleuser vs. jupyter-labhub.

As per the doc the app launched can be controlled through JUPYTERHUB_SINGLEUSER_APP.

  • Default -> SingleUserLabApp is launched
  • JUPYTERHUB_SINGLEUSER_APP=notebook.notebookapp.NotebookApp -> SingleUserNotebookApp is launched

See also this thread.

# default
docker run -it --rm -p 8888:8888 \
      jupyter/base-notebook start-singleuser.sh \
      | grep SingleUserLabApp
# [I 2022-01-19 16:43:54.025 SingleUserLabApp mixins:615] Starting jupyterhub single-user server version 2.0.2
# NotebookApp
docker run -it --rm -p 8888:8888 \
      -e JUPYTERHUB_SINGLEUSER_APP=notebook.notebookapp.NotebookApp \
      jupyter/base-notebook start-singleuser.sh \
      | grep SingleUserNotebookApp
# [I 2022-01-19 16:44:25.061 SingleUserNotebookApp mixins:615] Starting jupyterhub single-user server version 2.0.2

@mathbunnyru
Copy link
Member

My two cents:

  • Decide if we want to use this solution
    👍 , it looks really good to me.

  • Decide if we want to perform additional control to check if JUPYTER_CMD holds a proper value
    👎
    I think this is too much.
    It will not look good in bash and if docs are gonna be updated, I think a very small amount of people will encounter the problem.
    Also, the message jupyterlub-singleuser: command not found is quite easy to debug.

  • Decide if we want to keep a deprecation notice about the JUPYTER_ENABLE_LAB flag
    👍
    I think we should warn if this variable is set at all and it doesn't matter what it is set to.
    And this variable will be always ignored.

Also, please, add Improve tests coverage as a part of your to-do list.

@romainx
Copy link
Collaborator Author

romainx commented Jan 20, 2022

@mathbunnyru totally agree with you. I will continue this way.
Just to be sure on the point below. Do you mean that I should add it to the todo list just to check it or do you think there is room for improvement?

Also, please, add Improve tests coverage as a part of your to-do list.

👌 got it maybe you want to add a test case when JUPYTERHUB_API_TOKEN and RESTARTABLE are set -> I will add it.

Thanks !

@romainx romainx marked this pull request as ready for review January 20, 2022 08:42
Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

Please, grep JUPYTER_ENABLE_LAB - if there is something left, let's get rid of it.

I will also update the tag 33add21fab64 when this gets merged and built.

docs/using/common.md Outdated Show resolved Hide resolved
@mathbunnyru
Copy link
Member

And I do actually like the table which you added in your first message: #1575 (comment)

I think it's worth to add it to the docs as well.

@romainx
Copy link
Collaborator Author

romainx commented Jan 20, 2022

@mathbunnyru I made the changes requested. Please check if it's Ok for you.

Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

This looks awesome.

Let's wait for @consideRatio and then give it a go.

README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Beautiful! Thank you so much for your thorough work to make this change @romainx!

Naming of environment variables

One thing that I think is a bit problematic is the naming of our environment variables in general. They aren't named to indicate they influence this project's scripts.

I'm considering if we could try to do something about that to help people find their way to this projects documentation to learn more about the environment variable that they otherwise may not be sure what it does.

Overall I'd love to see that this project would prefix its environment variables, but, we would need to deprecate many old variables if we wanted to do that anyhow.

Example situation

Someone configures their installation of a JupyterHub Helm chart in a k8s cluster to start the servers differently, which works, because they use a jupyter/docker-stacks based image.

singleuser:
  extraEnv:
    JUPYTER_CMD: notebook

After this, someone copies configuration to a situation when they are not using an image derived from a docker-stacks based image, and voila - they are confused what's going on etc.

Naming of environment variables - conclusion

I'm okay with merging this as it is. I wanted though to raise the question on what you think about prefixing the new environment variable with DOCKER_STACKS_ as an initial step towards making our environment variables be prefixed like that in general.

@romainx
Copy link
Collaborator Author

romainx commented Jan 20, 2022

@consideRatio thanks a lot 😄! and this is a very good remark.
If @mathbunnyru is OK with that, I think it's better to change it now, it's not a problem for me.
If we stick with this prefix, it's better to do it before releasing a new variable that we will have to deprecate.
So let's go for?

  • JUPYTER_CMD -> DOCKER_STACKS_JUPYTER_CMD 🤔 not too long?

Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

I'm ok with whatever env variable you choose :)

docs/index.rst Show resolved Hide resolved
@romainx
Copy link
Collaborator Author

romainx commented Jan 20, 2022

@mathbunnyru and @consideRatio I think I'm done with this PR 😌.
I've renamed the variable.
I let you merge it!
Thanks for your thoughtful reviews 🙏

I will just update the issue to reflect what has been done and it will be 👌.

@romainx romainx mentioned this pull request Jan 20, 2022
3 tasks
@mathbunnyru mathbunnyru merged commit b418b67 into jupyter:master Jan 20, 2022
@mathbunnyru
Copy link
Member

Done. Welcome back, @romainx :)

@romainx romainx deleted the fix-1217-1 branch January 21, 2022 05:49
unkcpz added a commit to aiidalab/aiidalab-docker-stack that referenced this pull request Nov 28, 2022
The new jupyter/minimal-notebook image uses lab CMD to run the jupyter server, which was introduced by jupyter/docker-stacks#1575. It provides DOCKER_STACKS_JUPYTER_CMD to set how to run it.
The lab (full-stack) is for AiiDAlab only and should start with notebook mode. I pin it as an environment variable to start the jupyter backend.
Since we have the new jupyterhub, the `NOTEBOOK_ARGS` can be used to set the notebook arguments.
danielhollas pushed a commit to aiidalab/aiidalab-docker-stack that referenced this pull request Nov 28, 2022
The new jupyter/minimal-notebook image uses lab CMD to run the jupyter server, which was introduced by jupyter/docker-stacks#1575. It provides DOCKER_STACKS_JUPYTER_CMD to set how to run it.
The lab (full-stack) is for AiiDAlab only and should start with notebook mode. I pin it as an environment variable to start the jupyter backend.
Since we have the new jupyterhub, the `NOTEBOOK_ARGS` can be used to set the notebook arguments.
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.

Jupyter Notebook deprecation
5 participants