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

Slow build test and push #414

Closed
unkcpz opened this issue Nov 10, 2023 · 14 comments · Fixed by #439
Closed

Slow build test and push #414

unkcpz opened this issue Nov 10, 2023 · 14 comments · Fixed by #439

Comments

@unkcpz
Copy link
Member

unkcpz commented Nov 10, 2023

The build, test, and push to ghcr.io for each PR is still slow, the reason is the image is pushed to github artifacts.
It can be accelerated by pushing to ghcr.io without uploading the artifacts.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 10, 2023

Hi @danielhollas, I forgot to mention one thing in the meeting, the artifact was introduced for another reason that push to ghcr.io will fail if the PR is from the forked repo. It is the limitation of using GITHUB_TOKEN, using artifact does not have this problem.

Using artifact by downloading and load to docker should be easy as well if the instruction is provided. I think maybe the better solution is deprecate upload to ghcr.io for every PR but only upload to artifact, and after that pop an automatic comment telling people how to use the image from pr, just like the code coverage.
But this means the CI time is still slow.

Also pinning @yakutovicha for opinion.

@danielhollas
Copy link
Contributor

Honestly I think we can live with the fact that the upload will not work for forks, it's not like this repo needs external contributors. As is clear from the slowness, the artifacts were not really designed for such large upload, ghcr.io is the proper place for images. Also now that we made the images public, docker pull from ghcr.io is straightforward.

@yakutovicha
Copy link
Member

Honestly I think we can live with the fact that the upload will not work for forks, it's not like this repo needs external contributors. As is clear from the slowness, the artifacts were not really designed for such large upload, ghcr.io is the proper place for images. Also now that we made the images public, docker pull from ghcr.io is straightforward.

I think it is a good idea to not let external contributors push to ghcr.io cause during the build time they might execute an arbitrary code just by making a PR.

so I agree with @danielhollas here.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 14, 2023

Okay, makes sense to me as well. I 'll work toward that direction.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 16, 2023

image

Just find this. You deprived my excuse to play chairs during working time.

@danielhollas
Copy link
Contributor

Hahaha, now it all makes sense! : 😹

@danielhollas
Copy link
Contributor

danielhollas commented Dec 12, 2023

I noticed another thing - I think we should only upload to Dockerhub on the main branch, not PR. Dockerhub should not contain transient dev images.

EDIT: Scratch that, I was hasty, pushing to Dockerhub seems to be skipped on PRs.

@unkcpz
Copy link
Member Author

unkcpz commented Dec 12, 2023

😉 Yes, It is skipped. There is sha-XXX tag, but it corresponds to released versions.
But I'd regard it as a "mild" reminder for me to work on the issue 🤷

@unkcpz
Copy link
Member Author

unkcpz commented Dec 15, 2023

So with #419, do we still want to go with removing upload-artifact? Since the current solution works fine, maybe we can live with it?

@danielhollas
Copy link
Contributor

While the artifact upload is now significantly faster, I think we should still do this, and reasonably soon. There is still a lot of extra steps that we could remove and save a lot of time (2-3minutes at least, perhaps more). But the more important reason imo is that the current solution is overly complex and I worry it would be a pain to maintain. We should imo go back to original design and push to ghcr right after building and testing the image (perhaps even before testing, as was done before).

@danielhollas
Copy link
Contributor

I noticed that we now have a lot of duplicate tags on Dockerhub that contain OS/ARCH, such as amd64-aiida-2.4.3 and arm64-aiida-2.4.3. Those are completely unnecessary since they are both already part of aiida-2.4.3. I suspect this is another unintended side-effect of using artifacts as intermediate steps.

image

@danielhollas
Copy link
Contributor

Started working on this in #439

@unkcpz
Copy link
Member Author

unkcpz commented Mar 21, 2024

I noticed that we now have a lot of duplicate tags on Dockerhub that contain OS/ARCH, such as amd64-aiida-2.4.3 and arm64-aiida-2.4.3.

Using artifact is not exactly the source of problem. See https://github.com/aiidalab/aiidalab-docker-stack/blob/main/.github/workflows/docker-merge-tags.yml, it first upload both archs image to registry and merge them after.
Two options, 1) just using artifacts to store the images and then upload after merge 2) delete those after merge action is success.

@danielhollas
Copy link
Contributor

  1. Definitely not artifacts, I really want to get rid of those.
  2. I think we should avoid to upload them to dockerhub in the first place. My plan is to upload to ghcr.io first, and merge can happen later if needed.

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 a pull request may close this issue.

3 participants