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

Remove build and publish base and base-with-services to registry #412

Closed

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Oct 16, 2023

After aiidateam/aiida-core#6080, the use cases of base and base-with-services are all covered. I propose to remove those for simplicity. It also saves the CI build resource and time.

  • how to arrange the tests.
  • readme.

@unkcpz unkcpz marked this pull request as draft October 16, 2023 12:38
@unkcpz unkcpz marked this pull request as ready for review October 16, 2023 12:47
@unkcpz unkcpz force-pushed the devops/xx/deprecate-base-base-with-services-publish branch from 78c45c6 to fe44d7d Compare October 16, 2023 12:49
@danielhollas
Copy link
Contributor

@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.

@unkcpz
Copy link
Member Author

unkcpz commented Oct 18, 2023

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.

@danielhollas
Copy link
Contributor

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?
over 1h time is brutal, compared to what we had before. Is this all coming from the arm64 builds? Are we reusing all the caches properly?

@unkcpz
Copy link
Member Author

unkcpz commented Oct 18, 2023

over 1h time is brutal, compared to what we had before.

It is 1h for all images not for one, and not all are run in parallel because if lab is failed it is pointless to run full-stack. Originally all builds ran in parallel and produced 4 images.
Without the changes in this PR, it will finally generate 8 images, 4 for amd64 and 4 for arm64.
If you look at the time (https://github.com/aiidalab/aiidalab-docker-stack/actions/runs/6533815664?pr=412), every image build+test+uplead costs 10~15mins. AMR64 and AMD64 runs in parallel.
It is sad that the upload of artifacts is slow and uses 50% of the time, which is a GitHub action issue (actions/upload-artifact#199) and they aren't keen to solve it.

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.

Are we reusing all the caches properly?

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.

@danielhollas
Copy link
Contributor

Interesting, thank you for the write-up!
The last comment in that issue seems to imply that things should significantly improve in the upcoming v4 release of the artifact-upload action, so we should keep an eye on that.

Copy link
Contributor

@danielhollas danielhollas left a 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.

@unkcpz
Copy link
Member Author

unkcpz commented Oct 21, 2023

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.

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 base-with-services was used at that time?
In fact, the full-stack also can be used with JupyterLab as a server engine, only need to change the default server engine when deploying (and JupyterLab is the default actually). I pinning @mbercx who deployed the Azure services for previous tutorial, do you find base image is still useful if full-stack/lab can cover the use case (Need to mention a downside I can foresee is the full-stack/lab contains /home/jovyan/apps which is not useful and confusing maybe?).

Another option to accelerate the build time is make base/base-with-serveces and lab/full-stack build in parallel, downside is if the base is break the lab/full-stack will still build and waste some resources but I think it is acceptable.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 9, 2023

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.

@yakutovicha
Copy link
Member

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.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 9, 2023

@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.

@danielhollas
Copy link
Contributor

danielhollas commented Nov 9, 2023 via email

@unkcpz
Copy link
Member Author

unkcpz commented Nov 9, 2023

into separate containers (e.g. separate container for pgsql) and use docker-compose to
run them.

make sense to me if there are potential use cases of those images. I close this PR.

@unkcpz unkcpz closed this Nov 9, 2023
@unkcpz unkcpz deleted the devops/xx/deprecate-base-base-with-services-publish branch November 9, 2023 15:56
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 this pull request may close these issues.

3 participants