Set PIP_USER in base image #437
Conversation
|
@unkcpz @mbercx it looks like docker daemon is not running on the ARM64 self-hosted runner. https://github.com/aiidalab/aiidalab-docker-stack/actions/runs/8348422726/job/22850363649?pr=437 |
|
Arm runners are running again! |
Yeah, sorry about that! Not sure why my work station keeps on shutting down from time to time. 🤷 |
Somehow there was an issue on the colima to start the aiidalab docker instance, and I delete the profile and restart. I think it bring the issue #432 to high priority. |
That's true. |
| ENV PIP_CONSTRAINT=/opt/requirements.txt | ||
| ENV PIP_CONSTRAINT /opt/requirements.txt | ||
| # Ensure that pip installs packages to ~/.local by default | ||
| ENV PIP_USER 1 |
There was a problem hiding this comment.
I don't get why this will make the pip install to ~/.local, can you point me to the pip documentation?
There was a problem hiding this comment.
Yeah, sorry, this is actually equivalent to providing --user on the CLI.
You can set any pip argument via envvar by capitalizing it and pre-fixing with PIP_
There was a problem hiding this comment.
Thanks @danielhollas Looks all good, one minor request on the comment.
|
@unkcpz thanks for taking a look. I've pushed two new tests for the new pip behaviour. If you agree, I'll merge this and release a new latest version. |
|
@unkcpz friendly ping 😊 |
There was a problem hiding this comment.
All good, thanks for working on this @danielhollas
I had one question on the pip_install fixture.
| def pip_install(aiidalab_exec, nb_user): | ||
| """Temporarily install package via pip""" | ||
| package = None | ||
|
|
||
| def _pip_install(pkg, **args): | ||
| nonlocal package | ||
| package = pkg | ||
| return aiidalab_exec(f"pip install {pkg}", **args) | ||
|
|
||
| yield _pip_install | ||
| if package: | ||
| aiidalab_exec(f"pip uninstall --yes {package}") |
There was a problem hiding this comment.
I remember I mentioned somewhere there is an issue that the uninstallation will only remove the package but not the dependencies. Is this fixture solve that?
There was a problem hiding this comment.
Good point, it doesn't. I am not sure if this is a problem in practice: Here I only use it for a package that I specifically chosen to have zero dependencies. But it would be good to at least document this.
I think it was a slightly bigger issue when running the integration tests and installing actuall AiiDAlab apps.
There was a problem hiding this comment.
I think it was a slightly bigger issue when running the integration tests and installing actuall AiiDAlab apps.
You mean we should not test the QE-app or AWB in the full-stack image? That what I proposed but you said you were to comfortable with a image that not working with QeApp.
Can we make it a nightly test for QeApp and AWB that runs periodically?
There was a problem hiding this comment.
But it would be good to at least document this.
Yep, I'll add a comment on it.
There was a problem hiding this comment.
Can we make it a nightly test for QeApp and AWB that runs periodically?
I don't think that is very developer friendly, I'd much rather now about issues rightaway.
I will note however that originally these app-installation tests were completely separate from the others and marked as "integration" tests, they specifically were not supposed to block merging. We should go back to that, I'll do that as part of #439, or as a separate PR after #442 is merged, which is a pre-requisite for this work.
Ensure that pip installs to ~/.local by default, instead of /opt/conda. Other minor changes: * Do not pin pip version and always upgrade to latest, as recommended by pip maintainers. f there are any breaking changes in the future, they should be caught by the integration tests. * Install appmode from PyPI * Cleanup custom logo setup * pytest: Make --variant a required parameter * Simplify aiidalab_exec fixture usage * Move test_pip_check to common tests
Ensure that pip installs to
~/.localby default, instead of/opt/conda.Couple other changes:
--variantparameter of pytest requiredCloses Set PIP_USER=true in the image #435