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

Install python into a venv #718

Merged
merged 17 commits into from
Dec 21, 2023
Merged

Install python into a venv #718

merged 17 commits into from
Dec 21, 2023

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Nov 2, 2023

Decisions to be made:

  • Where do we set the appropriate env variables (VIRTUAL_ENV and PATH)? They need to be set for install_python.sh to work correctly. I've set them in the binder image for now, but it should probably be set on a more base image. This is a no-op if install-python.sh is not called anywhere.

TODO:

  • Update NEWS (can be done once everything else is finalized)

Ref #670

@yuvipanda
Copy link
Contributor Author

Created this draft mostly to see what CI says!

eitsupi pushed a commit that referenced this pull request Nov 2, 2023
Discovered while working on
#718
@yuvipanda yuvipanda force-pushed the venv branch 3 times, most recently from 07af31c to 484342f Compare November 2, 2023 18:16
@yuvipanda yuvipanda marked this pull request as ready for review November 2, 2023 18:18
@yuvipanda
Copy link
Contributor Author

ok, this is ready for a first round of review :)

dockerfiles/binder_devel.Dockerfile Show resolved Hide resolved
scripts/install_python.sh Show resolved Hide resolved
stacks/devel.json Show resolved Hide resolved
- Continues to use python and venv from Ubuntu LTS repositories,
  so they are supported as with everything else that is gotten
  from apt (see rocker-org#670 (comment))
- Doesn't currently change any permissions, so present behavior
  is preserved. However, in the future, we should probably change
  ownership so end users can install packages in there at runtime
  (see rocker-org#670 (comment))
- Sets the VIRTUAL_ENV environment variable to path of the venv
  we create. This is what the `source activate` script does,
  and Reticulate also looks for this to discover which
  python to use (see point 4 of https://rstudio.github.io/reticulate/articles/versions.html#order-of-discovery)
- Sets up PATH appropriately, so python and python3 refer to
  what is in our venv. This, along with the previous step, ensures
  same behavior as users typing `source ${VIRTUAL_ENV}/bin/activate`
  without actually having to do that, preserving end user behavioral
  semantics.
- Remove the explicit symlink of python3 -> python, as venv handles
  this automatically.

Decisions to be made:

- Where do we set the appropriate env variables (VIRTUAL_ENV and
  PATH)? They need to be set for `install_python.sh` to work
  correctly. I've set them in the binder image for now, but it
  should probably be set on a more base image. This is a no-op if
  `install-python.sh` is not called anywhere.

Ref rocker-org#670
So it sets PATH appropriately
@eitsupi
Copy link
Member

eitsupi commented Nov 28, 2023

What is the current status of this?

@yuvipanda
Copy link
Contributor Author

@cboettig gave me some feedback via slack that I need to dig into. @cboettig can you put them here too?

@cboettig
Copy link
Member

cboettig commented Dec 2, 2023

Thanks @yuvipanda !

Yup, I think the main thing is considering changing the permissions on /opt/venv from root to something else. (In the R site library we put these in the staff group, allowing any user added to that group to update or add packages to the shared site library. Not sure how nicely pip plays with group-based vs user-based permissions though? In my use/testing of this image I've just been making /opt/venv owned by the default user, https://github.com/boettiger-lab/nasa-topst-env-justice/blob/main/.devcontainer/Dockerfile#L16

A second more minor issue is that under this setup, the default python now points to /opt/venv/bin/python as intended, but if a user calls pip install instead of python -m pip install, they still get /usr/bin/pip instead of /opt/venv/bin/pip, which creates a gotcha that it would be better to avoid if possible??

@yuvipanda
Copy link
Contributor Author

A second more minor issue is that under this setup, the default python now points to /opt/venv/bin/python as intended, but if a user calls pip install instead of python -m pip install, they still get /usr/bin/pip instead of /opt/venv/bin/pip, which creates a gotcha that it would be better to avoid if possible??

Hmm, since there should be a pip in /opt/venv/bin/pip, if /opt/venv/bin is in $PATH, that should mean pip should call that pip by default, right? Is that not what's happening?

@yuvipanda
Copy link
Contributor Author

Making /opt/venv be owned by staff seems reasonable to me. If it works for R, my intuition is that it will also work for Python!

@cboettig cboettig self-requested a review December 20, 2023 20:46
@cboettig
Copy link
Member

@eitsupi ok just tested and this looks ready to merge to me!

stacks/devel.json Show resolved Hide resolved
@yuvipanda
Copy link
Contributor Author

Yay that's awesome, thank you @cboettig!

fi
# Make the venv owned by the staff group, so users can install packages
# without having to be root
chown -R rstudio:staff /opt/venv
Copy link
Member

Choose a reason for hiding this comment

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

Don't hard-code the username. There is no guarantee that the user exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eitsupi in install_texlive.sh I see:

NON_ROOT_USER=$(getent passwd "1000" | cut -d: -f1)

and this is used as username. Should I do that?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a bad reason not to be the root user?
If so, please do so.

However, in that case, I don't think it will work if the uid is anything other than 1000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, given I was trying to replicate what happens in R, I've set this to match what is done to $R_HOME/site-library

Copy link
Member

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

The tests seem passed. Thanks!

@eitsupi
Copy link
Member

eitsupi commented Dec 20, 2023

Could you update the NEWS file?

@yuvipanda
Copy link
Contributor Author

I always seem to struggle with updating NEWS but gave it a shot!

@eitsupi
Copy link
Member

eitsupi commented Dec 21, 2023

Looks good.

The CI failure is notthing to do with changes in this PR, so merging.

@eitsupi eitsupi merged commit 28085cc into rocker-org:master Dec 21, 2023
31 of 33 checks passed
@yuvipanda
Copy link
Contributor Author

Awesome! Thanks a lot, @eitsupi! Will this trigger a rebuild of the images?

@eitsupi
Copy link
Member

eitsupi commented Dec 23, 2023

Sorry, my review was not thorough enough (i.e. this should not have been merged yet), the fix for the stack file is completely incomplete here.
We had to update it manually for all versions.
See the issue #739.

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.

3 participants