-
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
Remove build and publish base and base-with-services to registry #412
Conversation
78c45c6
to
fe44d7d
Compare
@unkcpz can you please comment on how much total build time this saves? My gut feeling would be that it does not save that much since these images needs to be built anyway, and that's the expensive step, publishing should not be that expensive. But maybe I am missing something. |
Compare between two new CI runs:
I'd say save quite much. But as you said the image needs to be built anyway for lab/full-stack. The time saved is mostly from the slow upload docker image as an artifact step, which even takes a longer time than building the image. |
That is strange. Is this true for both amd64 and arm runners, or only on of them? |
It is 1h for all images not for one, and not all are run in parallel because if The artifact is essential to separate the image build and different arch combining so can not be skipped if we want to keep a clear separation of different arch/images.
There is no cache used (the artifacts are reused but not github action cache ). I think no cache should be used since every change we make potentially influences the final image and we want to get it tested. |
Interesting, thank you for the write-up! |
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.
I am unsure here, partly perhaps because I am not that familiar with the new aiida-core image and what exactly it covers. I get the benefits of not publishing unneeded images, but am unsure if we are not getting rid of useful functionality. I can imagine for example that the base-with-services
image that includes jupyter stack might be beneficial for tutorial / demo purposes.
I would like at least @yakutovicha to weigh in. Not sure who else might be using these images? @unkcpz feel free to ping somebody else who you think might be qualified to make this decision.
I did not look at the changes here in too much detail but they look fine to me at a glance.
Thanks for pointing this out, I didn't consider this. If I remember correctly during AiiDA tutorial the student is running tutorial in a JupyterLab environment. Thus I guess Another option to accelerate the build time is make |
As mentioned in the AiiDAlab, @danielhollas mentioned the artifact upload is not necessary, which I didn't realize. I 'll try it and that'll save a lot build/test time for the images. |
So is there anything for me to review at the moment? If you still need to work on the PR, then I prefer to have a look afterwards. Maybe make it a draft also. |
@yakutovicha for this PR it tries to remove the release of |
Note that there was a plan to decouple the various services into separate
containers (e.g. separate container for pgsql) and use docker-compose to
run them. Which might overall help to manage the dependencies.
So I would be against removing base and base-with-services for now, I think
we should be able to speed up the build times in other way as discussed.
Dne čt 9. 11. 2023 15:44 uživatel Jusong Yu ***@***.***>
napsal:
… @yakutovicha <https://github.com/yakutovicha> for this PR it tries to
remove the release of base and base-with-services, I think it is not
necessary because I think hard and can not come up with cases that can not
covered by lab and full-stack.
—
Reply to this email directly, view it on GitHub
<#412 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACIY64NSG75ZKTBM333INX3YDT24TAVCNFSM6AAAAAA6CD4HCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGA3TMNJQGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
make sense to me if there are potential use cases of those images. I close this PR. |
After aiidateam/aiida-core#6080, the use cases of
base
andbase-with-services
are all covered. I propose to remove those for simplicity. It also saves the CI build resource and time.