-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactor CI workflow #439
Refactor CI workflow #439
Conversation
f5c712c
to
f437f7c
Compare
f437f7c
to
dce9a38
Compare
Thanks! |
Yes, tests should be run separately and in parallel. But the current build strategy is extremely wasteful, since we're not reusing the previous layers (e.g. the |
That is correct! Initially I want to keep things as separated as possible and notice in the end every build cost < mins so didn't put effort to combine the built processes back to one. |
@mbercx would you mind installing |
This reverts commit 57f04ac.
@unkcpz I've pushed a fix to $ docker images
aiidalab/full-stack 2024.1017.post145.dev0_8f8a30e 1b069e00e91e About a minute ago 2.54GB
aiidalab/lab 2024.1017.post145.dev0_8f8a30e af94ef50d9cc About a minute ago 2.19GB
aiidalab/base-with-services 2024.1017.post145.dev0_8f8a30e e6be8727a8d1 4 minutes ago 2.54GB
aiidalab/base 2024.1017.post145.dev0_8f8a30e dba20a49b4f7 34 minutes ago 2.16GB The image tag (determined by VERSION variable) is automatically generated from git commit. Otherwise I think I replied to all your comments, thank you for a detailed review! Once I get a green light from you I will merge and then make a release for the aiida v2.5 image. |
Thanks for the fix. This works for me locally. But the command README seems not work |
Before merge this can we make a release with aiida-core==2.5.1 first? |
This should now be fixed, can you try again? Note that the |
I've temporarily switched to buildjet runners (and you can see that the tests pass now @unkcpz). |
Make sense to me. And I think we really need to move to buildjet. |
@unkcpz can you approve so I can merge? (I could bypass it but don't want to :-) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. This is much clear! Thanks. @danielhollas (and I know how difficult to figure out all the details 😄 ) I'll let you write up for a detail commit message, 153 commits seems a legend PR.
Overview
The goal of the PR is twofold:
In the end, I mostly ended up reverting things back to where they were roughly around PR #372, while (hopefully) keeping the current modularity. Closes #414
Major changes
The main change here is to stop uploading Docker images as artifacts: Instead we upload them to ghcr.io straightaway. This already brings a lot of simplification and time reduction -- the only "disadvantage" is that the PRs cannot be open from forks, but as discussed in Slow build test and push #414, this is more of a feature rather than a bug.
The second approach I took was to reduce our reliance on the self-hosted ARM64 runner, which is a limited resource (can only run one job at a time). In this PR, the arm64 image is build on ubuntu GHA runner with the help of QEMU virtualization. This is a bit slower then building natively, but not by that much (12min versus 9min). The other big advantage of this is that it reduces a lot of complexity, since we don't need to create / manage manifests manually, and simply let the
docker/bake-action
take care of the multiplatform build and upload. We can re-evaluate this in a future PR.We first build the amd64 images and immediately test
full-stack
. This overall takes only 6 minutes, so any obvious issues should be caught early.I believe some of the improvements here should be ported to QEApp and aiida-core repos as well.
Final results
+352/-470+320/-470.Future work
main