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

[MRG] install notebook in its own env #651

Merged
merged 18 commits into from Apr 30, 2019

Conversation

@minrk
Copy link
Member

minrk commented Apr 25, 2019

leave the conda root env alone

adds conda activation to profile.d and ensures that we start with a login shell via the ENTRYPOINT

closes #567 by adding an ENTRYPOINT that enables conda integration

cc @betatim

jhamman and others added 6 commits Apr 4, 2019
leave the conda root env alone

adds conda activation to profile.d and ensures that we
start with a login shell via the ENTRYPOINT
@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Apr 25, 2019

I think this is going to result in a performance improvement, since it skips a whole install step (the Python switch), and creates a new env instead of updating the root env.

The result is an ENTRYPOINT that ensures we launch with a fully configured login shell (profile.d, bashrc are sourced, unlike before), including the shell integration in #567. This also means that our notebook env is fully activated, in case there are any conda activation hooks in that env.

This will also facilitate #637 because we don't care what version Python is in the conda root env

minrk added 3 commits Apr 25, 2019
rather than replacing our entrypoint
in tests/conda/simple

use pytest for better error reporting
@minrk minrk referenced this pull request Apr 25, 2019
- conda is not importable in the frontmost python
- sys.executable is in $NB_PYTHON_PREFIX (this was true before, but it's value has changed)
@minrk minrk force-pushed the minrk:conda-env branch from 412873f to fc417e8 Apr 25, 2019
@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Apr 25, 2019

Additionally, this moves the handling of start_script down a level, so that our ENTRYPOINT is always called. The behavior should be the same.

like other things, this was always the right thing to do,
but more important now that notebook prefix is not the same as CONDA_DIR
@minrk minrk force-pushed the minrk:conda-env branch from fc417e8 to 70d950c Apr 25, 2019
tests/conda/repo-path/verify Outdated Show resolved Hide resolved
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 26, 2019

I think some of the failures are because we are still using conda 4.5 and not 4.6. For example the --stack argument seems to not exist in 4.5. I think we can update to 4.6 in this PR as well.

At least for https://travis-ci.org/jupyter/repo2docker/jobs/524519120#L931-L940

@minrk minrk force-pushed the minrk:conda-env branch from 2d13fba to 2acf097 Apr 26, 2019
@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Apr 26, 2019

@betatim good call. I merged in #637 and bumped conda up to 4.6.14 (there is now a miniconda 4.6.14) with a refreeze. This should mean significant performance improvements because we skip two conda install calls: one for the upgrade of conda (short-lived because conda and miniconda versions match for now) and another for the Python switch (permanent).

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 26, 2019

I re-added the step to make the start script executable as I think that was the cause for some of the tests to fail. Maybe you can squash the two commits into one (I used the webinterface :-/)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 26, 2019

I think

@assert IJulia.kerneldir() == "/srv/conda/share/jupyter/kernels"
is the line that fails in the Julia tests.

@yuvipanda

This comment has been minimized.

Copy link
Collaborator

yuvipanda commented Apr 30, 2019

@minrk how do you feel about splitting up the ENTRYPOINT related stuff from here into its own PR? Will help me with #657 :D

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 30, 2019

I think this is basically ready to be merged (I will fix the broken test now) and then we can start a new PR/rebase #657. What do you think @yuvipanda?

@betatim betatim force-pushed the minrk:conda-env branch from 4af0ee3 to e2c7de8 Apr 30, 2019
@betatim betatim changed the title [WIP] install notebook in its own env [MRG] install notebook in its own env Apr 30, 2019
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 30, 2019

I propose we merge this now. All tests pass, it gets us unblocked on conda versions, and one step closer to always splitting notebook server and user's kernel env.

@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Apr 30, 2019

I’m happy to merge this now. Sorry I didn’t finish it earlier.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 30, 2019

No worries. We are IMHO doing a good job of not having to be in a hurry.

Thanks for sorting us out with a nice new codna env setup!

@betatim betatim merged commit 9099def into jupyter:master Apr 30, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 100% of diff hit (target 20%)
Details
codecov/project 90.74% (+0.02%) compared to 7681a6c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@minrk minrk deleted the minrk:conda-env branch Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.