Skip to content

Conversation

@mathbunnyru
Copy link
Member

@mathbunnyru mathbunnyru commented Dec 2, 2023

Describe your changes

This makes conda env activate correctly for the whole process, not just the terminals.
I think it is reasonable and desirable to have the same environment inside a terminal and a Jupyter Notebook.

Issue ticket if applicable

Fix: #2043
Fix: #1792

Checklist (especially for first-time contributors)

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests
  • I will try not to use force-push to make the review process easier for reviewers
  • I have updated the documentation for significant changes

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Dec 2, 2023

I tested it this way:

make build/docker-stacks-foundation
make build/base-notebook
docker run -it --rm -p 8888:8888 quay.io/jupyter/base-notebook

In both the terminal and inside Jupyter notebook the result is the same:

env | grep CONDA
CONDA_EXE=/opt/conda/bin/conda
CONDA_PREFIX=/opt/conda
CONDA_PROMPT_MODIFIER=(base) 
_CE_CONDA=
CONDA_SHLVL=1
CONDA_DIR=/opt/conda
CONDA_PYTHON_EXE=/opt/conda/bin/python
CONDA_DEFAULT_ENV=base

I then tested it with docker run -it --rm --user root -e NB_USER=test -p 8888:8888 quay.io/jupyter/base-notebook.
Still works the same (which is nice and expected).

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Dec 2, 2023

But having eval "$(conda shell.bash hook)" only in a sourced file creates another problem:
This hook sets PS1 env var and creates inline bash functions to make conda activate and conda deactivate work.

But people open new Jupyter Terminals in the Web UI, meaning these inline bash functions are lost.
PS1 is also lost, because .bashrc and some other files in Linux contain setting PS1.
It also means in the terminal conda activate and conda deactivate won't work at all in a Jupyter Terminal.
It's not nice at all.

This practically means that we will have to eval twice:

  1. in a sourced magical file (which this PR added) - it will activate base environment, set proper env variables for the process
  2. In the .bashrc - this will fix PS1 and add an ability to use conda (de)activate in Jupyter Terminals (keeping it in this PR).

I also don't think command and 2> /dev/null are needed :

  1. first one is not needed since no conda function exists while we haven't called hook yet (and even if it exists, it's gonna work just fine calling actual conda executable)
  2. 2> /dev/null is not needed - conda creates correct bash code and I would prefer it break than silently be ignored.

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Dec 3, 2023

I have no idea what's going on with the test here.
Moreover, it works fine under aarch64, so I can't even test it easily 😢

@mathbunnyru
Copy link
Member Author

Also tested the recipe: docker build -t test . -f custom_environment.dockerfile, seems to work well.
PS1 now is also correct.

@mathbunnyru mathbunnyru closed this Dec 4, 2023
@mathbunnyru mathbunnyru reopened this Dec 4, 2023
@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

@mathbunnyru I think the issue can be considered resolved as soon as

  1. the conda environment is activated for both JupyterLab (Terminal) and Jupyter Notebooks.
  2. env variables are identical for IPython (started from JupyterLab Terminal) and Jupyter Notebooks.
    • This might not be the case for Terminal related env variables, though.

P.S.: I have added an order of precedence (i.e. numbering) for startup hooks: b-data/jupyterlab-python-docker-stack@82da8b4
ℹ️ Useful, if someone builds on the Jupyter Docker Stacks and wants to add additional startup hooks.

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Dec 4, 2023

@mathbunnyru I think the issue can be considered resolved as soon as

  1. the conda environment is activated for both JupyterLab (Terminal) and Jupyter Notebooks.

  2. env variables are identical for IPython (started from JupyterLab Terminal) and Jupyter Notebooks.

    • This might not be the case for Terminal related env variables, though.

P.S.: I have added an order of precedence (i.e. numbering) for startup hooks: b-data/jupyterlab-python-docker-stack@82da8b4 ℹ️ Useful, if someone builds on the Jupyter Docker Stacks and wants to add additional startup hooks.

As a developer, I would like to agree - it seems to work, easy to implement and to change for a custom environment.
From a user perspective, I would like to be able to do conda (de)activate easily in terminal.
And having correct PS1 would be nice as well.

Order of precedence is a nice idea. Should I call the file something like 10-activate-conda-env.sh?
Maybe even 10activate-conda-env.sh.

$ ls /etc/apt/apt.conf.d/
01autoremove      10periodic      20apt-esm-hook.conf  20auto-upgrades  20snapd.conf         50unattended-upgrades  99needrestart
01-vendor-ubuntu  15update-stamp  20archive            20packagekit     50command-not-found  70debconf              99update-notifier

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

As a developer, I would like to agree - it seems to work, easy to implement and to change for a custom environment.

That's what issue #2043 is about.

From a user perspective, I would like to be able to do conda (de)activate easily in terminal. And having correct PS1 would be nice as well.

Yes, of course. And this is really nice to have.

Great job.

@mathbunnyru
Copy link
Member Author

Renamed to 10activate-conda-env.sh.

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

This PR should also resolve issue #1792.

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Dec 4, 2023

This PR should also resolve issue #1792.

Wow, that's a great catch, it definitely should 👍
Didn't expect this PR to be this good 😆
I will verify it actually does (just to be sure) and then will update the PR description, so the issue is closed when I merge the PR.

Tests seem to pass, so longer sleep probably helped.
@benz0li Could you please review this PR?

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

@mathbunnyru LGTM and should work as intended.

docker run --rm -u root -e HOME=/root -w /root -ti test bash and executing conda activate python310 (from the recipe) leads to

CONDA_EXE=/opt/conda/bin/conda
CONDA_PREFIX=/opt/conda/envs/python310
CONDA_PROMPT_MODIFIER=(python310) 
_CE_CONDA=
CONDA_SHLVL=2
CONDA_DIR=/opt/conda
CONDA_PYTHON_EXE=/opt/conda/bin/python
CONDA_DEFAULT_ENV=python310
CONDA_PREFIX_1=/opt/conda

I assume CONDA_SHLVL=2 and CONDA_PREFIX_1=/opt/conda does not bother anyone.

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

Starting the container with docker run --rm -p 8888:8888 -ti test, opening in the browser and executing env | grep CONDA returns

CONDA_EXE=/opt/conda/bin/conda
CONDA_PREFIX=/opt/conda/envs/python310
CONDA_PROMPT_MODIFIER=(python310) 
_CE_CONDA=
CONDA_SHLVL=4
CONDA_DIR=/opt/conda
CONDA_PYTHON_EXE=/opt/conda/bin/python
CONDA_DEFAULT_ENV=python310
CONDA_PREFIX_1=/opt/conda
CONDA_PREFIX_2=/opt/conda/envs/python310
CONDA_PREFIX_3=/opt/conda

CONDA_SHLVL=N and CONDA_PREFIX_{1..N-1} is a side effect that should not bother anyone.

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

@mathbunnyru With the current mechanism: Is it really a good idea that the custom environment is activated by default?

Notebook/Console: 'Python 3 (ipykernel)' and 'python310' (from the recipe) are then identical (i.e. python310).
ℹ️ I would have expected, that Notebook/Console's 'Python 3 (ipykernel)' still uses the base environment.

IMHO The Terminal should also default to base even though a custom (conda) environment is made available.
👉 The user can change to their custom environment any time using conda activate "${env_name}".

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

P.S.: I am not the right person to talk to when it comes to Conda envs.
ℹ️ My Jupyter docker stacks use just Python – no Conda / Mamba installed.
👉 If a user wants Conda / Mamba, they have to install it on user level themselves.

@mathbunnyru
Copy link
Member Author

Is it really a good idea that the custom environment is activated by default?

I think I can comment this code with some information.

@mathbunnyru
Copy link
Member Author

docker run -it --rm -p 8888:8888 --user root -e NB_USER=vicente test works for me with a new recipe, so I think we can close the issue you mention.

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

docker run -it --rm -p 8888:8888 --user root -e NB_USER=vicente test works for me with a new recipe, so I think we can close the issue you mention.

Please check import os; os.environ and import sys; sys.version for both Notebook/Console 'Python 3 (ipykernel)' and 'python310'.

Is this expected?

Cross reference: ipython/ipykernel#416

@mathbunnyru
Copy link
Member Author

docker run -it --rm -p 8888:8888 --user root -e NB_USER=vicente test works for me with a new recipe, so I think we can close the issue you mention.

Please check import os; os.environ and import sys; sys.version for both Notebook/Console 'Python 3 (ipykernel)' and 'python310'.

Is this expected?

I checked with commented activation (as it is now in the PR).

os.environ results are the same (except JPY_SESSION_NAME).
I think Jupyter Lab/Console doesn't change conda when it is launched, so the variables are all the same

sys.version - 3.11.6 in default Notebook/Console and 3.10.13 in python310.
Totally fine

from typing import Self

This snippet works for default Notebook/Console and doesn't work for py310 versions (and it shouldn't work).

I also checked that python3.11 packages are not leaking somehow to python310 - also works fine, alembic is available in default Notebook/Console and is not available in python310.

@benz0li
Copy link
Contributor

benz0li commented Dec 4, 2023

Totally fine

All fine; Agreed.

I think Jupyter Lab/Console doesn't change conda when it is launched, so the variables are all the same

Yes. And the Jupyter Docker Stacks project is not supposed to fix IPython.


But I am quite sure people will complain that the custom (conda) environment is not activated when running the Notebook/Console with its kernel.

@mathbunnyru
Copy link
Member Author

Nice, thanks for your help ❤️
I appreciate it.

I'll merge this when the tests pass.

@mathbunnyru mathbunnyru merged commit 3253fc3 into jupyter:main Dec 4, 2023
@benz0li benz0li mentioned this pull request Dec 5, 2023
4 tasks
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.

Conda environment not fully set in Jupyter New conda environment issue with another username

2 participants