Don't install pypi packages to ~/.local - #1337
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1337 +/- ##
=======================================
Coverage 74.52% 74.52%
=======================================
Files 117 117
Lines 7442 7442
=======================================
Hits 5546 5546
Misses 1896 1896
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There is something wrong with the aiidalab_qe_vibroscopy package. uv refuses to install it, erroring about RECORD file. I can install it via pip, but then |
|
Blocked on aiidalab/aiidalab-qe-vibroscopy#147 |
|
Hi @danielhollas , now should be ok. Can you see if it works by changing here the two github values of the first two packages (vibroscopy and muon), to be |
|
Thanks @mikibonacci. We're now running into another issue Do you know why the |
|
Hi @danielhollas, I don't know. This is however strange: in the Anyway, pinging @superstar54 and @edan-bainglass to see if they knows the reason of fixing the |
That's because in this PR, I am installing all the packages together in a single pip install invocation so the resolver rightfully complains that the dependencies cannot be resolved. In the previous version might work, but the python environment is not consistent (you can verify that by running |
The spglib pin has been added in #1282, but there's not comment about it in the PR. |
|
@edan-bainglass FYI the discussion here gives a small taste for the dependency issues I briefly mentioned on Friday. |
a6cf647 to
fcc53cd
Compare
|
@danielhollas well, at least your PR is passing the build phase - can't say the same for the rest of the current PRs 🥲 I'm trying a few things on your PR to check the error in integration tests. Feel free to force push over these commits later once the PR is ready to merge. Update@danielhollas what I'm trying to do here is to disable the AiiDAlab error handler during integration tests, so that we can see the error in the screenshot if one arises. The mechanism I tried didn't take for some reason. In any case, I find it a bit inconvenient to have to wait on the build process just to test integration - unclear why this was designed this way. In #1422, I decouple these tests. My intention was to iterate on the integration tests to uncover the bug. However, I see that they pass, so the failure is coming from your PR 🤔 Peripherally, it would be great to understand how one can run selenium tests locally 🙂 Final updateOkay, looking at the screenshot error, the issue is related to an incompatible plugin w.r.t recent changes (#1389). Pinging @AndresOrtegaGuerrero @mikibonacci @superstar54 as a reminder to update all plugins. |
36ad1a1 to
6723558
Compare
|
I would be careful with this one , we install in .local , in case you are working in the server and there is a restart then when the server back the user has to install things manually. Will this PR avoid this ? |
I am pretty sure you if you build the qeapp image locally, then you should be able to run the Selenium tests in the same way as in CI. @yakutovicha definitely has done that with AWB so might have more insight. Feel free to drive this PR forward and merge it, I will not have capacity to debug this further as QeApp is not really my area. I ran into issues with ARM build, which was failing due to euphonic package build so I needed to still install aiidalab-vibroscopy via pip. But that can be improved in a subsequent PR.
I don't think I understand this. The packages are installed in the image itself in the last step in Dockerfile, so they should always be available? But maybe I am missing some context here. But in any case, things like this need to be documented in the Dockerfile. @edan-bainglass were you able to manually test the QeApp image from this PR? |
|
@AndresOrtegaGuerrero to clarify, this change is about the QeApp image, which is used to power the demo server, and has everything (QeApp and all plugins) pre-installed. Perhaps what you meant is when QeApp is installed manually from the AppStore? In that case I agree and the dependencies need to be installed in ~/.local so that they persist across restarts. This PR doesn't change that. |
6723558 to
91b2499
Compare
Cool! I'll give it a try.
Since the current state of this PR already reduces the size sufficiently to pass the build test, maybe we merge it already as is. We can continue to iterate in future PRs. For now, I've opened #1423 to handle the plugin compatibility issue, which should resolve the failed test here. I've rebased this PR on top of it. Let's see. UpdateOkay, #1423 indeed resolves one issue. The other is handled by #1417 (merged). This PR is now rebased on top and passes all tests. @superstar54 great if you can review this, as you put significant work on the image 🙏 |
91b2499 to
f520bc7
Compare
| # NOTE: uv refuses the install aiidalab-qe-vibroscopy due to invalid RECORD file, | ||
| # pyproject.toml is likely invalid as it contains direct git dependency. |
There was a problem hiding this comment.
This has been fixed
| # NOTE: uv refuses the install aiidalab-qe-vibroscopy due to invalid RECORD file, | |
| # pyproject.toml is likely invalid as it contains direct git dependency. |
edan-bainglass
left a comment
There was a problem hiding this comment.
@danielhollas you can relace all instances of ${VIBROSCOPY_PKG} with aiidalab-qe-vibroscopy, as the published package is now up-to-date.
|
@edan-bainglass I believe this is now ready. I have some more cleanup in mind but I'd rather do it in a separate PR. |
|
This PR shaves off around 400Mb from the image ❯ podman images | head
REPOSITORY TAG IMAGE ID CREATED SIZE
ghcr.io/aiidalab/qe pr-1337 b5399f3c5fec 26 minutes ago 5.96 GB
ghcr.io/aiidalab/qe edge b43c0834dfba 7 days ago 6.38 GB |
edan-bainglass
left a comment
There was a problem hiding this comment.
Fantastic! LGTM! Thanks @danielhollas 🙏
#1337 was great for ephemeral deployments (i.e., demo server), but does not work for persistent ones (e.g., institutional). #1337 installs the app and its plugins into conda, instead of .local. However, existing instances prior to #1337 have the app and its plugins in .local. Though Python defers to .local for imports, this is not true for entry points. The app's plugin system is based on loading entry points, which triggers an entry point search across a plugin's dependencies, which are now duplicated between .local and conda. This prevents the use of the given plugin. This PR keeps most of the recent changes to the image but shifts the installation of the app and plugins to .local. This means they are tarred and untarred on home when the container is first deployed (fresh volume). For existing volumes, conflicts are only in version. No more entry point conflicts!
Based on #1423
This drops the image size by 300Mb, since
/opt/home.tarcontained unwanted packages installed in~/.local, whereas they should have been installed to/opt/conda. Using uv instead of pip should also speed up the build.