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

BREAKING: singleuser.cmd defaults to Docker image default #2138

Closed
wants to merge 3 commits into from

Conversation

manics
Copy link
Member

@manics manics commented Apr 8, 2021

This is breaking change. Instead of setting the start command to jupyterhub-singleuser it now defaults to whatever was set in the Docker image.

In addition this may (or may not) make the switch to JupyterLab 3 easier:

  • If docker-stacks switches the default to JupyterLab 3 we just need to bump the base singleuser image. We shouldn't have to worry about handling backwards compatibility since we just run whatever the default command is.
  • If we make the switch ahead of docker-stacks then we could perhaps open a PR in docker-stacks to extend the use of the JUPYTER_ENABLE_LAB environment variable from just start-notebook.sh to also work in start-singleuser.sh. The big advantage here is we could then switch to JupyterLab 3 by setting JUPYTER_ENABLE_LAB without breaking images using JupyterLab 2 or those without JupyterLab. Since it's just an env var if your Docker image command doesn't care about it nothing breaks.

This is breaking change. Instead of setting the start command to `jupyterhub-singleuser` it now defaults to whatever was set in the Docker image.
@consideRatio
Copy link
Member

So this will be breaking for anyone that relies on a docker image that doesn't have a startup command that automatically ensures we make use of a startup script that couples us with the JupyterHub. In practice, this would be images not based on jupyter/docker-stacks for example that are made without consideration for JupyterHub.

A missing piece of knowledge is that jupyterhub-singleuser is a script provided by installing jupyterhub see this in jupyterhub's setup.py file.

I'm leaning for supporting this and writing about it in the changelog, which can be done separately if we agree that this is an acceptable change. It certainly is a easy to patch back to previous behavior if it is breaking.

@manics
Copy link
Member Author

manics commented Apr 9, 2021

It sounds like we'll have to make a breaking change for the switch to JupyterLab 3, and if we switch to jupyter-server at a later date that's potentially another breaking change. My thinking was that by unsetting the property here Z2JH is no longer responsible for choosing the singleuser application. Instead we'll require that the singleuser Docker image provides a JupyterHub compatible server. I'm happy to work on some docs if we go ahead with this.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyterhub-spark-spark-config-sh-usr-local-bin-before-notebook-d/8932/4

@mriedem
Copy link
Contributor

mriedem commented Apr 27, 2021

Even if you remove the default in the helm chart here the default in jupyterhub is still jupyterhub-singleuser so won't that be used?

https://github.com/jupyterhub/jupyterhub/blob/a71823c5ab802e9beeb0f5d5f7e21571472b1901/jupyterhub/spawner.py#L485

/me answers his own question:

Nope, because that's overridden by kubespawner:

https://github.com/jupyterhub/kubespawner/blob/98bb5773f0d1c8e0c065c7259511ea38103eabe9/kubespawner/spawner.py#L400

I've just been trying to figure out how to get rid of these warnings on notebook app startup when using jupyterhub:

Apr 27 13:27:46 jupyter-5e18a4193f4a3f001127f809 notebook [W 2021-04-27 18:27:46.816 LabApp] 'ip' has moved from NotebookApp to ServerApp. This config will be passed to ServerApp. Be sure to update your config before our next release.
Apr 27 13:27:46 jupyter-5e18a4193f4a3f001127f809 notebook [W 2021-04-27 18:27:46.816 LabApp] 'port' has moved from NotebookApp to ServerApp. This config will be passed to ServerApp. Be sure to update your config before our next release.

We have this set in the hub image (though I'm not sure from where):

jovyan@hub-54b895fcf6-vn2kk:/$ printenv JUPYTERHUB_SINGLEUSER_APP
jupyter_server.serverapp.ServerApp

Using docker-stacks scipy-notebook pinned to 837c0c870545 so we have jupyterlab 3.0.5 and nbclassic 0.2.6 installed. docker-stacks puts config under /etc/jupyter/jupyter_notebook_config.py and /etc/jupyter/jupyter_server_config.py (though I'm not sure which is used). It seems like maybe I just need to wait for some of these upstream projects to align on making jupyterlab the default (z2jh, docker-stacks, jupyterhub) before worrying too much about those deprecation warnings.

@minrk
Copy link
Member

minrk commented Apr 28, 2021

Related, but possible tangent: I think it is a better approach for images with 'pre-launch' stages like the Jupyter docker stacks to implement the bulk of the startup in ENTRYPOINT instead of CMD (ref). This way it's ~always run, and the consequences of setting the command to launch are more what you expect them to be. This is what we do in repo2docker-built images, for exactly this reason.

@consideRatio
Copy link
Member

@minrk @yuvipanda what do you think about this suggested change?

I'm in favor of it. With this PR, I think we reduce the complexity in a good way.

I have struggled a lot with mapping out what @manics mapps out very clearly in the topic. When z2jh, kubespawner, and the Dockerfile, as well as scripts within the image are all doing things it becomes complicated. With this change, z2jh will no longer do something unless explicitly configured.

I'll go for a merge unless someone objects by friday evening!

@minrk
Copy link
Member

minrk commented Apr 30, 2021

👍. It's not clear to me what's best, but solving known problems is worth a shot. Matching kubespawner, at the very least, is a good thing. This also matches DockerSpawner's longstanding behavior.

@consideRatio pointed out the main case negatively affected by this - specifying images where jupyterhub is available will no longer work by default without also specifying the command, but for those the answer is always the same: jupyterhub-singleuser, which seems worth it.

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

I think I put this in when z2jh started (?) and it was a mistake. Unsetting is the right thing to do.

However, this is a heavy breaking change now - can we add more docs and a heavy bit of notice to the changelog? Happy to merge after that.

@manics
Copy link
Member Author

manics commented Apr 30, 2021

I'll definitely add some docs to this PR now that there's agreement for this change.

@manics
Copy link
Member Author

manics commented May 2, 2021

I can write a Discourse announcement too when this is merged.

@consideRatio
Copy link
Member

consideRatio commented May 2, 2021

@manics having thought a lot about KubeSpawner's cmd configuration in jupyterhub/kubespawner#493, I think this PR may be a very major breaking change after all :'(

The big difference is that we leave cmd unset, and by doing so, invoke a big difference in how things are treated by KubeSpawner when it comes to spawner.get_args() which in turn influence all of the following configuration as well - as this won't be passed now.

Configuration Resulting argument augmented to the spawner.cmd if spawner.cmd isn't blank
spawner.ip --ip=...
spawner.port --port=...
spawner.default_url --SingleUserNotebookApp.default_url=...
spawner.notebook_dir --notebook-dir=...
spawner.debug --debug
spawner.disable_user_config --disable-user-config
spawner.args <user defined args>

@manics
Copy link
Member Author

manics commented May 3, 2021

Good point. Though if we're switching to JupyterLab 3 I think we'll end up making breaking changes anyway.

@consideRatio
Copy link
Member

consideRatio commented May 4, 2021

@manics because switching to JupyterLab by default implies setting cmd to jupyter-labhub?

I'm currently very hesitant to merge this, feeling inactionable by the complexity of all the related issues. Do you have an action point to suggest? My concern is that we make a change here only to have a lot of users running into issues with default_url no longer working because get_args() no longer is augmented to the Dockerfile's CMD even the spawner.cmd and the Dockerfile's CMD are the same due to jupyterhub/kubespawner#493.

Related

Me when considering this complexity

image

@manics
Copy link
Member Author

manics commented May 4, 2021

The original motivation for me suggesting this change was to make it easier to configure the default singleuser application in Z2JH by delegating the choice to the Docker image instead of Z2JH, which in turn was motivated by the desire to switch to JupyterLab 3. It just so happens that I discovered the problem of start.sh being bypassed whilst investigating some forum posts, which I think is also a good justification for this change.

I think we need to consider this and JupyterLab 3 together. If we're prepared to make breaking changes to switch to JupyterLab 3 I think we should consider merging this PR since it helps to avoid future problems if we need to switch again (e.g. for jupyter server, or if something else replaces JupyterLab, or just to make it easier for people to cusomise their images). If we find a non-breaking way to switch to JupyterLab 3, or we decide it's too much trouble, that removes one of the drivers behind this change, and we should consider the pros and cons again.

@consideRatio
Copy link
Member

If we're prepared to make breaking changes to switch to JupyterLab 3 I think we should consider merging this PR since it helps to avoid future problems if we need to switch again (e.g. for jupyter server, or if something else replaces JupyterLab, or just to make it easier for people to cusomise their images).

I still don't have a clear mind about what switching to JupyterLab 3 as default would imply, but merging this PR standalone is something that makes me very uneasy at this point. If I would write the CHANGELOG message or respond to questions about that later i would communicate something like...

We removed the default value of jupyterhub-singleuser for singleuser.cmd, but with it being unset the singleuser.defaultUrl configuration won't work any more for very technical reasons too tiresome to explain for both of our sakes, so we recommend you to simply specify it again explicitly.

My current opinion

  1. We work towards freeing ourselves from augmenting the cmd with information but instead trying to rely on environment variables to pass additional information.
  2. We retain a truthy singleuser.cmd value such as jupyterhub-singleuser or jupyter-labhub and don't set it to None - as default_url etc will stop working by doing so.

@manics
Copy link
Member Author

manics commented May 5, 2021

I'll close this then, we can always reopen in future.

@manics manics closed this May 5, 2021
@consideRatio
Copy link
Member

consideRatio commented May 5, 2021

@manics thank you for exploring this, let us reopen as soon as we improve the CLI args situation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants