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

Create CI to build & push PyBaMM's Docker Images #3316

Merged
merged 21 commits into from
Oct 8, 2023

Conversation

arjxn-py
Copy link
Member

@arjxn-py arjxn-py commented Sep 9, 2023

Description

This PR aims to create a CI that builds all PyBaMM's docker images & then pushes them to DockerHub.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 9, 2023

Related to #3312
cc: @agriyakhetarpal @brosaplanella @Saransh-cpp

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 9, 2023

I'd also need help with setting up the docker secrets for this one.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Would building and pushing images in their separate jobs be a better way, rather than in a series of steps?

@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d02a78b) 99.58% compared to head (b059ebe) 99.58%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3316   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        19998    19998           
========================================
  Hits         19915    19915           
  Misses          83       83           

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

.github/workflows/docker.yml Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved
@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 9, 2023

Would building and pushing images in their separate jobs be a better way, rather than in a series of steps?

Can they run parallelly in case of separate jobs? I'd be happy to switch to separate jobs either way though 🙂
Thanks!

@agriyakhetarpal
Copy link
Member

Can they run parallelly in case of separate jobs?

Yes, they will run and upload images in parallel. We would need to check out the repository in each job, however. Since it would be developers who would be using the image, could you remove the sparse checkout and let it fetch the entire commit depth of the repository?

@agriyakhetarpal
Copy link
Member

Actually, multiple jobs might not be needed in this case, a matrix for the solver arguments should suffice after the check out step

@Saransh-cpp
Copy link
Member

I'd also need help with setting up the docker secrets for this one.

Do you want us to add them in the GitHub repo or do you want us to generate it from Dockerhub?

@arjxn-py
Copy link
Member Author

Do you want us to add them in the GitHub repo or do you want us to generate it from Dockerhub?

I'd need help with adding them to github repo, i can generate & send secrets to you on slack.

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Sep 22, 2023

Note: to push to Docker Hub we should use https://github.com/docker/build-push-action.

I noticed that they have some examples about setting up QEMU, we should be able to build both AMD64 and ARM64 images with that (I may take a look in another PR).

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @arjxn-py! Looks great! Should be ready to merge once the comment below is resolved -

.github/workflows/docker.yml Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member

Also, taking into consideration that we would further be pushing images on release candidates, releases, and possibly nightly releases too (not in this PR): I was wondering about storage limits on Docker Hub. @arjxn-py do you know if there are any? If so, we could push just the "All solvers" image because the other images would then be redundant, and document this change in the Docker installation page. If someone wants to build an image with a specific solver locally, we already have instructions on how to do that

@arjxn-py
Copy link
Member Author

arjxn-py commented Oct 2, 2023

storage limits on Docker Hub

As of my knowledge & research there's no storage limit for images on DockerHub (i.e. we can have as many tags we want) however there's an individual image size limit which we don't have to bother about as our image size is smaller.

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Oct 2, 2023

That's great, thanks for the info! Pushing all images should be fine then

@Saransh-cpp Saransh-cpp self-requested a review October 3, 2023 12:45
@Saransh-cpp Saransh-cpp merged commit e124663 into pybamm-team:develop Oct 8, 2023
31 of 33 checks passed
Comment on lines +67 to +72
- name: Build and Push Docker Image (With All Solvers)
uses: docker/build-push-action@v5
with:
context: .
file: scripts/Dockerfile
tags: pybamm/pybamm:latest
Copy link
Member

Choose a reason for hiding this comment

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

I think we did not tag the images properly. On https://hub.docker.com/r/pybamm/pybamm/tags I can see that this image was pushed with the latest tag, and

- name: List built images
run: docker images

- name: Build and Push Docker Image (Without Solvers)
Copy link
Member

Choose a reason for hiding this comment

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

this one also has the latest tag

Copy link
Member

Choose a reason for hiding this comment

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

the earlier one (with all solvers) should have had the all tag. I do have a local branch which fixes things up a bit for this workflow (including logging in and uploading these images in parallel jobs) and will write a PR soon, I might look into adding the multi-platform images there itself.

agriyakhetarpal added a commit to arjxn-py/PyBaMM that referenced this pull request Oct 9, 2023
pybamm-team#3316 fix discrepancy with tagging the images, add a step to append tags to GitHub output, add support for multi-architecture images by setting up QEMU.
@arjxn-py arjxn-py deleted the docker-ci branch February 21, 2024 07:09
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