-
Notifications
You must be signed in to change notification settings - Fork 363
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
Support Python 3.11 and upgrade jupyterhub from 1.5.0 to 3.1.1 #1239
Conversation
replaces `.py2` special-case with a general `separate_kernel_env` flag, currently triggered by requesting Python < 3.7. Python 3.6 and 3.5 are now treated as Python 2.7
now that it's a kernel-only environment
Thank you @minrk for working this! This LGTM from what I can tell but I struggle with understanding the consequences of things in repo2docker. There are consequences of staying behind as well though and would very much like to get this merged due to them. I've seen some test failures, so I restarted the test run in the PR and started new one in the master branch as well to compare. Still waiting for the outcome. |
This looks ok to me, as it primarily just adopts the mechanism we have for py2/3 and updates it to be 'is python new enough?'. I am not sure about the failing tests but otherwise good to me! |
0b0218e
to
d8c2995
Compare
Most of the failures were due to updated expectations of py35/36, expecting kernel Python to be first on $PATH, which is not the case when the two are separate. I've updated the tests to match the new behavior (explicitly test with kernel Python, rather than The Julia failure is not merely one of test expectation, because Julia 0.6 pins Python 3.5 due to compatibility issues between the associated old version of IJulia and updates to jupyter-core. I've set the $JUPYTER env to the kernel env when running with 0.6, which seems to fix the problem (I ran the test, as well as manually building the repo and launching a Julia notebook, which worked). 0.6 is also still our default Julia (when used with REQUIRE), which has been outdated now since the release of 0.7, in 2018. I don't want to change defaults in this PR, but maybe we should drop support for Julia 0.6 and/or deprecate REQUIRE altogether. I think it all works now (we'll see what CI says). |
`/usr/bin/env python` is no longer the kernel python
one last typo. optimistic 🤞 for CI this time |
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.
❤️ 🎉 nice work!!! I've reviewed the recent changes, and they LGTM!
I understand this as possibly breaking, but not so much, and that we don't overview the various situations so well. I figure its mainly an issue for users of an image with jupyterhub if so, where things starts working better if the server is jupyterhub 3 based suddenly, while it may also degrade if the jupyterhub is jupyterhub 1 based for exmaple?
I suggest we call this breaking.
I'm happy to go for a merge, do you consider this ok to be merged?
I think it's all set from my perspective. It's definitely breaking for Python 3.5/3.6 repos. Also, I guess the JupyterHub version upgrade is technically breaking, too, for folks using r2d to build JupyterHub images. Python 2.7, 3.7-3.9 users shouldn't see a difference. |
Edit by Erik:
continuation of #1196, implementing this suggestion.
Changes:
.separate_kernel_env
flag, instead of.py2
special case, currently triggered bypython < 3.7
and easy to update in the future.In testing likely user surprise, I ran a test with
postBuild
and Python 3.6, and noticed that $NB_PYTHON_PREFIX comes before $KERNEL_PYTHON_PREFIX on PATH. I know this is intentional, but it has consequences, because it decreases the reproducibility of certain postBuild actions (likely the least stable part of repo2docker) because things likepip
andjupyter
refer to the server environment, thoughconda install
is managed to install in the kernel environment. But changing it could mean common postbuild actions likejupyter labextension build
stop working because they are run against the wrong env. So there's not an obvious solution.I think probably a better and more expected experience for most users would be for $KERNEL_PYTHON_PREFIX to be first on PATH, and the server (when it's different) to be last. But we also need to make sure kernel-first doesn't cause problems when we are trying to launch the server, which is kinda the main thing we do here. I'd like to save that for a later discussion, if we can, but I understand if folks would rather solve it. As it is now, this PR is preserving behavior of server-first, only raising the question of whether that should change, not changing it.
I'd rather unblock long-delayed JupyterHub and Python updates soon, which unfortunately requires a solution to this 3.5/3.6 problem, and I think this is the best compromise.