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

Automatically build docker images for each Brian release #1503

Merged
merged 35 commits into from
Mar 11, 2024
Merged

Conversation

bdevans
Copy link
Contributor

@bdevans bdevans commented Feb 12, 2024

There is still a bit of tidying to do and in particular a few items to deal with but I thought I would put it up for you to see now.

TODO

  1. Create secrets for user and password
  2. Make workflow a job in the PyPI package build workflow
    • Ensure pypi is used for brian2 and brian2tools (now the workflows are merged this should be the case if the jobs are executed in serial)
  3. Check sudo works
  4. Tidy up comments

@mstimberg
Copy link
Member

Hi @bdevans, thanks for this, I will look into it in more details later. Just to check that I understand correctly: on running the docker container, it will start a jupyter lab instance that you can connect to via your browser?

@bdevans
Copy link
Contributor Author

bdevans commented Feb 13, 2024

Yes, that's right. Naturally the default behaviour can be changed to e.g. a bash prompt or it can be overridden as desired on any given run by passing the command to execute at the end of the run command (see the commented build and run commands at the top of the Dockerfile).

@bdevans bdevans changed the title [WIP] Automatically build docker images for each Brian release Automatically build docker images for each Brian release Mar 7, 2024
@bdevans bdevans marked this pull request as ready for review March 7, 2024 14:37
@bdevans bdevans requested a review from mstimberg March 7, 2024 14:37
@bdevans
Copy link
Contributor Author

bdevans commented Mar 7, 2024

I think this is ready to review. One last refinement would be to somehow get the version from pypi or the repository and tag the image with it (in addition to latest) so that there is an archive of every release made as a docker image.

@mstimberg
Copy link
Member

Hi @bdevans. I had a closer look and I agree we are almost there. I played around a bit with the docker file and there are a few minor ways to decrease the size of the image, I'll push them soon. One thing I realized was that the only reason we needed python3-dev and libgs-dev was because brian2 was built from source since we don't have a binary wheel for 3.12 on PyPI (yet). But then wondering about testing the build process without doing a release I realized that we should probably change the logic a bit: instead of pulling brian2 in with the other dependencies as part of the docker build, we can build it outside the container and copy the wheel into the container for installation. This way, our process would be comparable to the way we build pypi packages: we build the docker container for every commit, but only push it to the docker hub for releases. We don't have the equivalent of testpypi, but we can at least make the docker image available as an artifact on the GitHub action for developer testing. How does this sound?

@mstimberg
Copy link
Member

One last refinement would be to somehow get the version from pypi or the repository and tag the image with it (in addition to latest) so that there is an archive of every release made as a docker image.

We have access to the tag name, but I have to say I do not quite understand how the GitHub action currently decides on the tag to use. What would we have to change to push with a version tag – isn't this what the metadata action is already doing (if not, what is it doing 😊 ?)

@bdevans
Copy link
Contributor Author

bdevans commented Mar 7, 2024

One last refinement would be to somehow get the version from pypi or the repository and tag the image with it (in addition to latest) so that there is an archive of every release made as a docker image.

We have access to the tag name, but I have to say I do not quite understand how the GitHub action currently decides on the tag to use. What would we have to change to push with a version tag – isn't this what the metadata action is already doing (if not, what is it doing 😊 ?)

Sorry, you're right - I had already set it up and was confusing myself! 🙃

@mstimberg
Copy link
Member

Um, regarding my proposal of building the wheel outside the docker build process: how does the buildx with the two platforms actually work in practice? Will it run the docker build twice with different architectures? So we'd have to create two wheels and use the correct one from within the container?

@bdevans
Copy link
Contributor Author

bdevans commented Mar 7, 2024

Hi @bdevans. I had a closer look and I agree we are almost there. I played around a bit with the docker file and there are a few minor ways to decrease the size of the image, I'll push them soon. One thing I realized was that the only reason we needed python3-dev and libgs-dev was because brian2 was built from source since we don't have a binary wheel for 3.12 on PyPI (yet). But then wondering about testing the build process without doing a release I realized that we should probably change the logic a bit: instead of pulling brian2 in with the other dependencies as part of the docker build, we can build it outside the container and copy the wheel into the container for installation. This way, our process would be comparable to the way we build pypi packages: we build the docker container for every commit, but only push it to the docker hub for releases. We don't have the equivalent of testpypi, but we can at least make the docker image available as an artifact on the GitHub action for developer testing. How does this sound?

I agree, we don't want to build from source - it would be better as a complete test of the user experience of installing the binary from pypi. If we do build for every commit, it might be useful to push that to a separate image e.g. briansimulator/brian:dev that is continually overwritten or perhaps a separate repo.

On the subject of dependencies, should it be libgs-dev or libgsl-dev (which is installed in the devcontainer) or both?

@bdevans
Copy link
Contributor Author

bdevans commented Mar 7, 2024

Um, regarding my proposal of building the wheel outside the docker build process: how does the buildx with the two platforms actually work in practice? Will it run the docker build twice with different architectures? So we'd have to create two wheels and use the correct one from within the container?

Good question. It emulates the hardware and builds both in parallel as far as I understand it. So, yes, I believe we would need wheels for both arm64 and amd64, although if we build them and push them to pypi, then install them from there, it should pick the pick architecture automatically, right?

@mstimberg
Copy link
Member

I agree, we don't want to build from source - it would be better as a complete test of the user experience of installing the binary from pypi.

Hmm, but the user won't build the docker image? What I mean is: why should we make the round trip (build wheel, upload it to pypi, then download it for the docker build), instead of directly providing the wheel to the docker build process? For me, the disadvantage of using pypi is that it means we can never build/test docker images on branches, until we merge them into main. It is not very likely that a change in Brian breaks something that makes the docker build fail, but not our other tests, but in principle it could happen, right? It would be nice to notice this before merging a PR.

@bdevans
Copy link
Contributor Author

bdevans commented Mar 7, 2024

I agree, we don't want to build from source - it would be better as a complete test of the user experience of installing the binary from pypi.

Hmm, but the user won't build the docker image? What I mean is: why should we make the round trip (build wheel, upload it to pypi, then download it for the docker build), instead of directly providing the wheel to the docker build process? For me, the disadvantage of using pypi is that it means we can never build/test docker images on branches, until we merge them into main. It is not very likely that a change in Brian breaks something that makes the docker build fail, but not our other tests, but in principle it could happen, right? It would be nice to notice this before merging a PR.

Ah I see. Yes ok it seems reasonable to cut pypi out of the loop then for the benefit of building wheels on every branch. You could build the wheels as a separate job or I could make the Dockerfile a multi-stage build where the wheels are built in the first stage (with the *dev libraries installed) then copied into the next stage resulting in a lighter image.

@mstimberg
Copy link
Member

We are already building the wheels on the GitHub action just before, so I think we can simply reuse them :) Let me see whether I can quickly put together something.

COPY dist /tmp/dist

# Install Brian2 and recommended packages
ARG TARGETARCH
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be preferable to get the architecture from the image directly with something like: dpkg --print-architecture?

Copy link
Member

Choose a reason for hiding this comment

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

Just to make clear that there is no misunderstanding: this is not an argument that you need to set, it is coming from docker (https://docs.docker.com/build/building/variables/#multi-platform-build-arguments). I understood that I have to declare them like this (but I didn't actually test it):

These arguments are useful for doing cross-compilation in multi-platform builds. They're available in the global scope of the Dockerfile, but they aren't automatically inherited by build stages. To use them inside stage, you must declare them:

But there might be another way that is easier – ideally something that uses the same terminology as the pip wheel file names…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough - six of one, half a dozen of the other I guess 🤷‍♂️

@bdevans
Copy link
Contributor Author

bdevans commented Mar 8, 2024

Do we need libgsl-dev?

Copy link
Member

@mstimberg mstimberg left a comment

Choose a reason for hiding this comment

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

Hi @bdevans, from my side this now all looks good – we'll only see whether a version gets correctly tagged when we release it, though…
I updated the README with instructions to build everything locally. But I think it is very Linux-specific, no idea whether it will work like this for you. Still, good enough from my side for now :) Feel free to merge if all looks good to you, too.

@mstimberg
Copy link
Member

Do we need libgsl-dev?

Ah, forgot about this – I think we do to run GSL integration on C++ standalone, but I won't be able to look into it before Monday.

@bdevans
Copy link
Contributor Author

bdevans commented Mar 11, 2024

I just did a last bit of cosmetic tidying but I think this is good to go!

@bdevans bdevans merged commit 9872c74 into master Mar 11, 2024
26 of 54 checks passed
@bdevans bdevans deleted the docker_images branch March 11, 2024 11:43
@mstimberg
Copy link
Member

FYI: the automatic push of the release image (with release version and as latest) to docker hub worked like a charm 🥳
https://hub.docker.com/repository/docker/briansimulator/brian/general

@bdevans
Copy link
Contributor Author

bdevans commented Mar 15, 2024

60% of the time, it works every time... 😉

@mstimberg
Copy link
Member

I'm always worried about these things that you cannot really test beforehand… If you look at the number of times I had to push out a X.0.1 release a few hours later 😅 Very happy to see that everything seems to have worked flawlessly this time.

@bdevans
Copy link
Contributor Author

bdevans commented Mar 15, 2024

A relief indeed and congrats on the new release! 😄

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.

2 participants