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

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

Closed
wants to merge 1 commit into from

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Jul 2, 2024

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.

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.28%. Comparing base (e2d3d93) to head (11c4c26).
Report is 40 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #762   +/-   ##
=======================================
  Coverage   68.28%   68.28%           
=======================================
  Files          45       45           
  Lines        4143     4143           
=======================================
  Hits         2829     2829           
  Misses       1314     1314           
Flag Coverage Δ
python-3.10 68.28% <ø> (ø)
python-3.9 68.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@unkcpz
Copy link
Member

unkcpz commented Jul 3, 2024

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 #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.

@unkcpz unkcpz removed their request for review July 3, 2024 13:42
@superstar54
Copy link
Member

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
Copy link
Contributor Author

@superstar54 please have a look at this PR aiidalab/aiidalab-docker-stack#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
Copy link
Contributor Author

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
Copy link
Contributor Author

Closed in favour of #761

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.

3 participants