aiidalab / aiidalab-qe

AiiDAlab App for Quantum ESPRESSO
https://aiidalab-qe.readthedocs.io/
MIT License
11 stars 14 forks source link

Dockerfile: Add --no-user to pip install #762

Closed danielhollas closed 2 months ago

danielhollas commented 3 months ago

This is an important fix for the Docker build since version v2024.1017 of the aiidalab full-stack image, which added the --user argument to pip install by default.

@superstar54 it seems that the aiidalab_qe app dependencies were being installed in ~/.local instead of /opt/conda. This may have influenced your testing of your tared-home image, as it would blow up the home size.

CC @unkcpz

NOTE: As an alternative to this PR you can merge #761 and use uv instead of pip which speeds up the docker build by 1 minute.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 68.28%. Comparing base (e2d3d93) to head (11c4c26).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #762 +/- ## ======================================= Coverage 68.28% 68.28% ======================================= Files 45 45 Lines 4143 4143 ======================================= Hits 2829 2829 Misses 1314 1314 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-qe/pull/762/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/aiidalab/aiidalab-qe/pull/762/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `68.28% <ø> (ø)` | | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-qe/pull/762/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `68.31% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

unkcpz commented 3 months ago

I think if we go with the zip and decompress solution, the time won't influenced by where it installed.

This may have influenced your testing of your tared-home image, as it would blow up the home size.

But this is true! @superstar54 can you add the change to your PR and check the image size change?

As an alternative to this PR you can merge https://github.com/aiidalab/aiidalab-qe/pull/761 and use uv instead of pip which speeds up the docker build by 1 minute.

If we go with prepare a ready to use home and decompress it for user, we don't need pre-install the dependencies, true? Then I think it is better to install things as what will exactly happened as aiidalab-launch install.

superstar54 commented 3 months ago

Hi @danielhollas , thanks for the PR.

it seems that the aiidalab_qe app dependencies were being installed in ~/.local instead of /opt/conda.

Could you give more details on why it is?

danielhollas commented 3 months ago

@superstar54 please have a look at this PR https://github.com/aiidalab/aiidalab-docker-stack/pull/437

It essentially changed the pip default -- when you run pip install in the container, it behaves as if you run pip install --user. So if you don't want that behaviour, you need to specify pip install --no-user. LMK if you need more info.

danielhollas commented 3 months ago

If we go with prepare a ready to use home and decompress it for user, we don't need pre-install the dependencies, true? Then I think it is better to install things as what will exactly happened as aiidalab-launch install.

Yeah, you don't need to, though I guess it still helps a bit since it will make home much smaller, and hence untar/decompress should be faster (and the overall image size smaller as well).

danielhollas commented 2 months ago

Closed in favour of #761