GHA: Docker build refactor #6396
Conversation
danielhollas
force-pushed
the
docker-refactor
branch
from
69866d7 to
d9443f5
Compare
|
I'd suggest to merge this to get the docker build fixed, and deal with the issues of forks in subsequent PR
Sure, I’ll review it.
Sent from Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Daniel Hollas ***@***.***>
Sent: Wednesday, May 22, 2024 4:23:58 AM
To: aiidateam/aiida-core ***@***.***>
Cc: Jusong Yu ***@***.***>; Mention ***@***.***>
Subject: Re: [aiidateam/aiida-core] GHA: Docker build refactor (PR #6396)
@unkcpz<https://github.com/unkcpz> This is ready for review. I'd suggest to merge this to get the docker build fixed, and deal with the issues of forks in subsequent PR.
—
Reply to this email directly, view it on GitHub<#6396 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACDMFROBVSEUUQ42X4IPSC3ZDP6T5AVCNFSM6AAAAABHZUMFOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTG42TCNRTGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
There was a problem hiding this comment.
Thanks @danielhollas. Only a nitpick, all looks good. Let's try the test and publish CI after merge.
| - name: Set Up Python 🐍 | ||
| if: ${{ startsWith(inputs.runsOn, 'ubuntu') }} | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.11' | ||
| cache: pip |
There was a problem hiding this comment.
I assume this will fail for arm64? But anyway let's test it after merge.
There was a problem hiding this comment.
It would fail, that's why this step is skipped :-)
Co-authored-by: Jusong Yu <[email protected]>
| REGISTRY_USERNAME: ${{ secrets.DOCKER_USERNAME }} | ||
| REGISTRY_TOKEN: ${{ secrets.DOCKER_TOKEN }} | ||
| needs: [arm64-build] | ||
| runsOn: buildjet-4vcpu-ubuntu-2204-arm |
There was a problem hiding this comment.
Hi @danielhollas, I just talk to the guy from warpBuild, and it has $8 per month for free, may try once by just change to nerver mind, let's merge it first and we test it later.warp-ubuntu-latest-arm64-2x?
There was a problem hiding this comment.
Sure, I'll give it a go. I am just going out the office, so will do later today or tomorrow. (Or feel free to push to this branch if you can).
|
Hmm, after merging main the builld failed. Will have a look. |
|
@unkcpz hmm, the warp arm64 runner doesn't seem to pick up the job. Do you need to somehow activate the trial period?
|
|
@sphuber this is ready to merge. |
|
The Docker Images / test-amd64 build is failing. Is this expected and of any concern? |
|
@sphuber I restarted the test and it passed. It seems to be somewhat flaky, not sure why. I suspect sometimes the image download stalls for some reason (I blame GHA). We should keep an eye on how often it happens, and whether we need to investigate further. |
.github/workflows/ci-code.yml
Outdated
| @@ -2,7 +2,7 @@ name: continuous-integration-code | |||
|
|
|||
| on: | |||
| push: | |||
| branches-ignore: [gh-pages] | |||
There was a problem hiding this comment.
Is this intentional? Now the normal unit tests workflow will only run on merge to main right and no longer on push to branches?
There was a problem hiding this comment.
The unit tests will still run on pull_request event.
I made this change for this PR in particular since I was opening the pull request from origin, and so the status checks were all duplicated. This is what happens for PRs from forks as well, essentially wasting CI compute imo.
So this change would only make a difference to you if you care about running tests on push before you open your PR. Given that you can always open a draft PR I feel like this use case is not as important, but I am happy to revert this in case you think differently.
Thanks for spotting this! I totally forgot I did this here. Sorry about that.
There was a problem hiding this comment.
I agree that in most cases, having it run doubly is not ideal.
So this change would only make a difference to you if you care about running tests on push before you open your PR. Given that you can always open a draft PR
This is the critical point for me though. In many cases, I rely on running the CI on my fork. I don't really like the concept of draft PRs and so I rarely open them. If the work is not ready, there is rarely a reason to open a (draft) PR. The downside for me as a reviewer is that it adds a lot of noise. I look at the PR page often to see what needs to be done and when. Having a lot of open draft PRs where everytime I have to assess whether to consider or skip it, adds noise. So I don't really like the idea that users will be forced to open them just to get the CI to run.
I could be convinced otherwise, but I think this discussion shows that we probably shouldn't sneak the change into this PR, so for now please remove it and I will merge
There was a problem hiding this comment.
No problem, done.
This should both increase speed and robustness of the build, main due to: * Not uploading image as artifacts. (this does have a side-effect that the build can't happen from forks, maybe we can workaround that) * Build multiplatform images together Co-authored-by: Jusong Yu <[email protected]>
This is an analogous refactor that I did over at aiidalab-docker-stack, please see that PR for details. aiidalab/aiidalab-docker-stack#439
tl;dr This should both increase speed and robustness of the build, main due to: