search

opensafely / research-template

The template for new research projects that use the OpenSAFELY framework.
MIT License
16 stars 13 forks source link

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

Closed Jongmassey closed 6 months ago

Jongmassey commented 6 months ago

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

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

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.