rocker-org / rocker-versioned2

Run current & prior versions of R using docker. rocker/r-ver, rocker/rstudio, rocker/shiny, rocker/tidyverse, and so on.
https://rocker-project.org
GNU General Public License v2.0
409 stars 173 forks source link

Install python into a venv #718

Closed yuvipanda closed 9 months ago

yuvipanda commented 11 months ago

Decisions to be made:

TODO:

Ref https://github.com/rocker-org/rocker-versioned2/issues/670

yuvipanda commented 11 months ago

Created this draft mostly to see what CI says!

yuvipanda commented 11 months ago

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

eitsupi commented 10 months ago

What is the current status of this?

yuvipanda commented 10 months ago

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

cboettig commented 10 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

yuvipanda commented 9 months ago

Yay that's awesome, thank you @cboettig!

eitsupi commented 9 months ago

Could you update the NEWS file?

yuvipanda commented 9 months ago

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

eitsupi commented 9 months ago

Looks good.

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

yuvipanda commented 9 months ago

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

eitsupi commented 9 months ago

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.