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

Move docker stack build from docker stack to aiidalab-qe #449

Merged
merged 19 commits into from
Sep 21, 2023
Merged

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Aug 23, 2023

Attempt to solve aiidalab/aiidalab-docker-stack#406

I adapt most of the codes I used in aiidalab-docker-stack, except in this repo, I call docker buildx bake directly instead of using doit script.
It reveals that with this implementation, the integration test can be easily run with the image build. Also, provide a preview container on ghcr for immediate test of new feature implementation.

  • Move the notebook test to using the built image directly.

@unkcpz unkcpz marked this pull request as draft August 23, 2023 10:05
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a6ce2af) 70.45% compared to head (6323ec5) 70.45%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #449   +/-   ##
=======================================
  Coverage   70.45%   70.45%           
=======================================
  Files          42       42           
  Lines        2955     2955           
=======================================
  Hits         2082     2082           
  Misses        873      873           
Flag Coverage Δ
python-3.10 70.45% <0.00%> (ø)
python-3.8 70.50% <0.00%> (ø)
python-3.9 70.50% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/aiidalab_qe/app/configuration/pseudos.py 88.54% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@unkcpz unkcpz force-pushed the docker-stack branch 16 times, most recently from 1d06a63 to f272c90 Compare August 25, 2023 16:01
@unkcpz unkcpz marked this pull request as ready for review August 25, 2023 16:02
@@ -347,7 +347,7 @@ def _get_pseudos_family(self, pseudo_family: str) -> orm.Group:
.one()[0]
)
except exceptions.NotExistent as exception:
raise ValueError(
raise exceptions.NotExistent(
Copy link
Member Author

@unkcpz unkcpz Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This change is separated as a single commit 6c194c6. The exception needs to be propagated up and not raise, otherwise if the pseudopotential library not installed, the app will crash when structure selected and confirm.

Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wow, I wasn't expecting the PR to look so big.

Do we need all this complicated machinery? I thought we just made a tiny docker file + 2 bash scripts.

I believe it shouldn't be as complicated as the base repository where there are multiple stages/versions.

If you want, I can try to make a concurrent PR with what I have in mind.

@unkcpz
Copy link
Member Author

unkcpz commented Aug 28, 2023

Oh, wow, I wasn't expecting the PR to look so big.

I try to use all the state-of-art pr review feature from what I introduced to aiidalab-docker-stack, since we have it there, these codes are just duplicate some of it and create images not only for amd64 but images for arm64 and run tests on it.

If you want, I can try to make a concurrent PR with what I have in mind.

Sure, please try it. But I am doubt there is a simple one with all the useful features exists here, see the checkboxes below:

  • release arm64/amd64 and combine these into one tag. check docker-merge.yml
  • release for ghcr and for dockerhub as well when making new release. (using reusable workflow and pass architecture as an inputs, check docker-merge-tags.yml)
  • The PR can be reviewed in the ghcr. (I realize this is quite useful when reviewing PRs, the newly implemented features can quickly be seen by the reviewer.)
  • (Optional) Since the image is built in the PR, the integration tests can be run from inside without install the package from full-stack image. This not only save time for install the qe from conda and pseudo libraries which makes testing a real calculation possible (it is currently only selects structure and checks the selenium element of the next step), but it tests the image created directly rather than depending on the specific version of full-stack image.

@unkcpz
Copy link
Member Author

unkcpz commented Sep 21, 2023

Hi @yakutovicha, I deployed the macOS action runner on PSI Machine with Marnik, and all tests are passed. I believe to support ARM64 especially run on self-hosted runner on macOS the complicities here are difficult to avoid. But if your app doesn't need support arm64, then it is possible to have a light CI to build and test, you can do the test on your app.

Feel free to have a go-through on the implementation, if you have no objection I'll merge it tonight.

@unkcpz unkcpz force-pushed the docker-stack branch 8 times, most recently from 3566165 to f0e97fa Compare September 21, 2023 16:38
@unkcpz unkcpz merged commit edc7b56 into main Sep 21, 2023
19 checks passed
@unkcpz unkcpz deleted the docker-stack branch September 21, 2023 19:13
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.

None yet

2 participants