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.

Sign up for GitHub

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

Jump to bottom

Build & Push Docker Images on releases #3661

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft

Build & Push Docker Images on releases #3661

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f9ce65
Build & Push Docker Images on release
arjxn-py Dec 24, 2023
3cd6573
#3312 initial setup, configure release tags via name
agriyakhetarpal Dec 28, 2023
83fda19
Temporary changes to test tag generation
arjxn-py Dec 28, 2023
a269e9b
Skip login step temporarily
arjxn-py Dec 28, 2023
5b8b83d
Try using `github.ref_name` instead
arjxn-py Dec 28, 2023
bc0fa0b
Revert temporary changes
arjxn-py Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 29 additions & 2 deletions .github/workflows/docker.yml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

The entire file shows up as changed, possibly due to a difference in line endings schemes?

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ on:
push:
branches:
- develop
release:
types: [published]
Copy link
Member

Choose a reason for hiding this comment

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

I'm very rusty at dockerhub stuff at the moment, but IIRC, shouldn't we tag the "release" images with the git tag of the pybamm's release?

I mean - won't this overwrite the "latest" (develop) image on dockerhub with the "release" image, and then as soon as a new commit is pushed to develop, the "release" image will be overwritten by a new "latest" image?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm aware. We're still discussing atm about how the tags will be set since we want to label the image with either the PyBaMM version or the name of the release (since it contains the version) – the latter is easier to extract from the GitHub events API. Marking the PR as draft for now since it's not ready yet

Copy link
Member Author

Choose a reason for hiding this comment

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

shouldn't we tag the "release" images with the git tag of the pybamm's release?

Yes, you're right. We'll Push Docker Images Based on release tags only, that way we'll have a different image for each release but as @agriyakhetarpal mentioned we're discussing which Image should we push for release to keep it single and simple for each release (i.e. We're mostly in favor of image with IDAKLU or ALL solver), We'd be happy to have your opinion on this @Saransh-cpp

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we're also discussing reducing the number of images we build at this time – the reason is that the latest image without any solvers is just a simple git clone command and doesn't make sense to include, the jax image can be superseded by pip install pybamm[jax] where wheels are currently available for all platforms, which makes the installation a matter of seconds. The images where there are any compilation steps present are odes, idaklu and subsequently all – all of them use the same SUNDIALS that has been compiled in .local/. My opinion is towards just pushing the all image to accommodate all solvers and no other images (regardless of pushes to develop or releases). We run the image builds on commits to the main branch and the arm64 image workflow runs for ~40 mins due to QEMU emulation, so we are not constrained about time or size either. @arjxn-py, could you open a new issue about this?

Copy link
Member

Choose a reason for hiding this comment

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

Also, we've been mixing conda and virtualenv environments in the image – while that does work without issues now, it's not really a good idea to do so, even in late 2023. I would suggest switching to using a manylinux2014-compliant base image (i.e., with glibc) in the not-so-long term; it will essentially be the process of containerising PyBaMM from the ground up once again, but the added experience in this area for all of us in the past months would mean it can be done pretty quickly.

Copy link
Member Author

@arjxn-py arjxn-py Dec 26, 2023 •
edited

Choose a reason for hiding this comment

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

could you open a new issue about this?

Sure, I'm up for making the all image as standard one if we do not consider size. Although, this is also to be noted that latest, jax & odes image with all also manages the environment & installation from source which I feel is different than pip install pybamm[extras]. Happy to open a new Issue about the same and have a discussion there 🙂
See #3666

Copy link
Member

Choose a reason for hiding this comment

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

Yeah ultimately it doesn't make sense for the jax image to have SUNDIALS pre-installed. The [odes] extra is the only one where we don't have wheels on PyPI so we have to compile from source – perhaps they are not publishing because of copyleft licensing issues...

Copy link
Member Author

@arjxn-py arjxn-py Jan 6, 2024 •
edited

Choose a reason for hiding this comment

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

won't this overwrite the "latest" (develop) image on dockerhub with the "release" image, and then as soon as a new commit is pushed to develop, the "release" image will be overwritten by a new "latest" image?

Guess I missed this while answering previously 😬 , Sorry.
No, this wont overwrite the previous release image as two different release images would have two different tags and as soon as tags are different the images are distinct. Maybe you can notice here how there are different tags and different images.


jobs:
build_docker_images:
Expand Down Expand Up @@ -34,6 +36,7 @@ jobs:
password: ${{ secrets.DOCKERHUB_TOKEN }}

- name: Create tags for Docker images based on build-time arguments
if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' && github.event_name != 'release'
id: tags
run: |
if [ "${{ matrix.build-args }}" = "No solvers" ]; then
Expand All @@ -48,7 +51,24 @@ jobs:
echo "tag=all" >> "$GITHUB_OUTPUT"
fi

- name: Create tags for Docker images based on build-time arguments (release)
if: github.event_name == 'release' && github.event.action == 'published'
id: tags-release
run: |
if [ "${{ matrix.build-args }}" = "No solvers" ]; then
echo "tag=${{ github.event.release.name }}-latest" >> "$GITHUB_OUTPUT"
arjxn-py marked this conversation as resolved.
Show resolved Hide resolved
elif [ "${{ matrix.build-args }}" = "JAX" ]; then
echo "tag=${{ github.event.release.name }}-jax" >> "$GITHUB_OUTPUT"
elif [ "${{ matrix.build-args }}" = "ODES" ]; then
echo "tag=${{ github.event.release.name }}-odes" >> "$GITHUB_OUTPUT"
elif [ "${{ matrix.build-args }}" = "IDAKLU" ]; then
echo "tag=${{ github.event.release.name }}-idaklu" >> "$GITHUB_OUTPUT"
elif [ "${{ matrix.build-args }}" = "ALL" ]; then
echo "tag=${{ github.event.release.name }}-all" >> "$GITHUB_OUTPUT"
fi

- name: Build and push Docker image to Docker Hub (${{ matrix.build-args }})
if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' && github.event_name != 'release'
uses: docker/build-push-action@v5
with:
context: .
Expand All @@ -57,5 +77,12 @@ jobs:
push: true
platforms: linux/amd64, linux/arm64

- name: List built image(s)
run: docker images
agriyakhetarpal marked this conversation as resolved.
Show resolved Hide resolved
- name: Build and push Docker image to Docker Hub (${{ matrix.build-args}}) (Release)
if: github.event_name == 'release' && github.event.action == 'published'
uses: docker/build-push-action@v5
with:
context: .
file: scripts/Dockerfile
tags: pybamm/pybamm:${{ steps.tags-release.outputs.tag }}
push: true
platforms: linux/amd64, linux/arm64