-
Notifications
You must be signed in to change notification settings - Fork 192
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
New docker image based on pure ubuntu and gracefully handle services #6080
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #6080 +/- ##
==========================================
+ Coverage 79.64% 79.71% +0.07%
==========================================
Files 546 546
Lines 39811 39987 +176
==========================================
+ Hits 31704 31870 +166
- Misses 8107 8117 +10 ☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
d4f6c9d
to
564c2c0
Compare
I push the image to ghcr.io, to test the PR image, just run:
|
.docker/README.md
Outdated
All commands `build`, `tests`, and `up` will use the locally detected platform and use a version tag based on the state of the local git repository. | ||
However, you can also specify a custom platform or version with the `--platform` and `--version` parameters, example: `doit build --arch=amd64 --version=my-version`. | ||
|
||
You can specify target stacks to build with `--target`, example: `doit build --target base --target base`. |
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.
The doit
not only used for local build and test, but it is the command run on the CI action directly.
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.
To see how it works in CI, you can run this command locally from .docker
folder.
|
||
RUN mamba install --yes \ | ||
--channel conda-forge \ | ||
postgresql=${PGSQL_VERSION} && \ |
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.
postgresql is installed from conda forge to avoid the version may not exist from apt repo.
# Install erlang. | ||
RUN apt-get update --yes && \ | ||
apt-get install --yes --no-install-recommends \ | ||
erlang \ |
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.
It is tricky to install the correct RMQ version because the erlang version needs to compatible with the final rabbitmq version as shown in https://www.rabbitmq.com/which-erlang.html
.docker/base-with-services/s6-assets/init/postgresql-prepare.sh
Outdated
Show resolved
Hide resolved
Is this fully ready for final review @unkcpz . I notice the tests are not yet passing? |
7b2976a
to
cd02a89
Compare
Hi @sphuber, yes it is ready to review. I also rebase the commits to every commit are specific to one task. Edit: now the test failed for install CI test and seems caused by pymatgen? |
The The third failure is not (necessarily) related to that. I have seen it before in my branch where I am introducing the |
Here is the culprit: materialsproject/pymatgen@118c245 |
edfbdd5
to
00620b9
Compare
Yes, done! But seems #6109 still fails with those three tests. |
No, there is just one test remaining. The two related to pymatgen are fixed, but as I already explained above, the third one is not related and I have no clue what causes it. |
I see. But it is strange to me why pre-commit CI in #6109 is not failed but failed in this PR? It also has the old pymatgen version installed (python3.9). |
It also failed in my PR originally, but I fixed it. The problem is that since we are now using API of two different versions, |
a9191d2
to
df6aece
Compare
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.
Thanks @unkcpz . I have added some initial comments. I have also commented on the AEP with a question on the image names. Let's discuss that there first, before we start changing it here.
Minimal working version of the basic docker image build locally base image can run First working with-service stack adapt dodo script to build for correct arch Use mamba image back again workable s6-overlay solution Properly tear down the aiida daemon Fixes CI test pass locally fix env variable in s6 scripts suppress dev version warning [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci push ghcr only when it is pull request form org upload artifact which is time consuming only for PR from org Add a CI action only for container build not run some container action from fork Add doc for the docker build and how to use the new docker stack Fixes the package permission pre-commit fix - pg log to home folder Fixes CI errors [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Update pyproject.toml Update .github/workflows/docker.yml exclude .docker files from yapf and pylint Update .docker/dodo.py With git, vim, ssh client pre-installed more doc on how to start the exist container Add .local/bin to PATH for packages install by user Pin PyYAML<5.3 Looks like the particular issue is caused by a new release of Cython which breaks PyYAML builds. yaml/pyyaml#724 yaml/pyyaml#724 Pinning PyYAML to earlier version seems to do the trick for now... Remove doit but use docker baked directly Rename image to aiida-core-base and aiida-core-with-services
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.
Great stuff, thanks a lot @unkcpz
The current Docker image provided with `aiida-core` depends on `aiida-prerequisites` as a base image. This image is maintained outside of the `aiida-core` repo, making it additional maintenance to keep it up to date when a new `aiida-core` version is released. In addition, the `aiida-prerequisites` image is no longer maintained because the AiiDAlab stack now depends on another base image. Finally, the `aiida-prerequisites` design had shortcomings as to how the required services, PostgreSQL and RabbitMQ, are handled. They had to be started manually and were not cleanly stopped on container shutdown. An AEP was submitted to add two Docker images to `aiida-core` that simplifies their maintenance and that improve the usability by properly and automatically handling the services. See the AEP for more details: https://aep.readthedocs.io/en/latest/009_improved_docker_images/readme.html Cherry-pick: 9e80834
The current Docker image provided with `aiida-core` depends on `aiida-prerequisites` as a base image. This image is maintained outside of the `aiida-core` repo, making it additional maintenance to keep it up to date when a new `aiida-core` version is released. In addition, the `aiida-prerequisites` image is no longer maintained because the AiiDAlab stack now depends on another base image. Finally, the `aiida-prerequisites` design had shortcomings as to how the required services, PostgreSQL and RabbitMQ, are handled. They had to be started manually and were not cleanly stopped on container shutdown. An AEP was submitted to add two Docker images to `aiida-core` that simplifies their maintenance and that improve the usability by properly and automatically handling the services. See the AEP for more details: https://aep.readthedocs.io/en/latest/009_improved_docker_images/readme.html Cherry-pick: 9e80834
Supercede #6072
Details are described in aiidateam/AEP#39
I commend on the PR for some technical details and the link to s6 toolkit.
The PR is from
aiidateam
directly so the push to ghcr.io can be triggered. I also set the arm64 test only triggered from PR fromaiidateam
to prevent the cost from malicious PRs from the forked repository.