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

Remove --user from pip install in postCreate.sh #126

Merged
merged 1 commit into from
May 22, 2024

Conversation

Jongmassey
Copy link
Contributor

@Jongmassey Jongmassey commented May 20, 2024

venv is activated prior to this script execution following this change to the dockerilfe so --user installation no longer allowed and errors reported by @lucyb and replicated by @Jongmassey .

Rather than remove --user and install opensafely into the virtualenv, use a direct path to the system pip3 and thus decouple it from the virtualenv entirely.

@Jongmassey Jongmassey self-assigned this May 21, 2024
Copy link
Member

@iaindillingham iaindillingham left a comment

Choose a reason for hiding this comment

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

Could you describe the problem more clearly in both the commit message and the PR description? I followed the link to opensafely-core/research-template-docker#26, but there's no description of that problem, either.

This is a good example of an issue where the solution is small relative to the description of the problem. However, the description of the problem will be invaluable to our future selves. I can imagine future me running git blame on .devcontainer/postCreate.sh, finding the commit message, finding the PR description, and thinking "Why on earth did Jon do that?"

From my perspective, I think the problem is that a venv is automatically activated in each shell in a dev container.1 When pip install --user ... is executed within a venv, it fails:

ERROR: Can not perform a '--user' install. User site-packages are not visible in this virtualenv.

So, the problem is that .devcontainer/postCreate.sh is executed in a shell in a dev container with an active venv. It fails at the call to pip install.... The solution is to modify the call to pip install..., changing a --user install to a local install.

Describing the problem more clearly highlights the tight coupling between research-template and research-template-docker. Is there any way we could avoid assuming that a venv is activated? Could we use the absolute path to pip?

Footnotes

  1. This is because source /opt/venv/bin/activate appears in /home/rstudio/.bashrc. It's important to realise that rstudio is the user account on the dev container; it's the user account that runs RStudio, but also the user account that runs VS Code.

@Jongmassey
Copy link
Contributor Author

clearly highlights the tight coupling between research-template and research-template-docker

This is something we've discussed when deciding to put the latter in its own repo. In our current state with no tests that test across both it's very easy to make breaking changes.

Is there any way we could avoid assuming that a venv is activated?

I think this is a reasonable assumption given the entry in .bashrc. IIRC you raised an issue that the venv activation wasn't very robust prior to this change - is there a method of activation you'd prefer? (bearing in mind we need to account for both the CLI and the MS Python extension)

Could we use the absolute path to pip?

/usr/local/bin/pip3 install opensafely --user works

I've got no preference as to installing in the venv vs user-installing. If there's a good reason for one of the other please say and I'll do it the way you want. That or just push a suitable commit to this PR (or a new one).

@iaindillingham
Copy link
Member

We should use the absolute path to pip, as this weakens the coupling. Assuming that the venv exists and is activated is two links in the coupling; assuming that the venv exists is one link in the coupling.

Please, though, describe the problem more clearly in both the commit message and the PR description. Doing so will be invaluable to our future selves.

Following opensafely-core/research-template-docker#26
which added venv activation into .bashrc for the rstudio user (the default
user for our codespaces/devcontainer configuration), calling `pip3` would
call /opt/venv/bin/pip3. Within a virtualenv, the `--user` option is not
allowed and users reported this error. Instead we decouple the installation
of opensafely-cli from the venv so it is always available regardless of
virtualenv state.
@Jongmassey Jongmassey force-pushed the Jongmassey/venv-pip-fix branch from efddf07 to 06cbe3c Compare May 21, 2024 20:24
@Jongmassey Jongmassey requested a review from iaindillingham May 21, 2024 20:25
@Jongmassey Jongmassey merged commit bc24007 into main May 22, 2024
1 check passed
@Jongmassey Jongmassey deleted the Jongmassey/venv-pip-fix branch May 22, 2024 09:00
Jongmassey added a commit that referenced this pull request Jun 7, 2024
Introduced in error with #126
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.

2 participants