search

mesh-adaptation / animate

Anisotropic mesh adaptation toolkit for Firedrake
MIT License
5 stars 0 forks source link

Automatically update Docker image #47

Closed jwallwork23 closed 5 months ago

jwallwork23 commented 8 months ago

Closes #43.

stephankramer commented 5 months ago

Could you say a bit more what you've done here. I can see it builds a firedrake container (docker/Dockerfile.firedrake) - but when does it do this - and does the rebuild of the container trigger a repeat of the test suite?

jwallwork23 commented 5 months ago

Could you say a bit more what you've done here. I can see it builds a firedrake container (docker/Dockerfile.firedrake) - but when does it do this - and does the rebuild of the container trigger a repeat of the test suite?

I'm still trying to figure out how to get this working. I'm somewhat of a novice regarding GitHub Actions. If you or @acse-ej321 have any insight I'd be very grateful! I think the current attempt tries to build the Docker container upon pushing to the PR (https://github.com/pyroteus/animate/pull/47/commits/b5ba4a4de288e8889900fa2a4e638f05a0f13643). I guess we actually want it to happen upon merging the PR? Or as a last step before merging? Open to ideas.

jwallwork23 commented 5 months ago

^Various guesses at how to get this working, no joy so far. Will have to read more into how this stuff works.

stephankramer commented 5 months ago

Same here - being somewhat of a novice with github actions - normally I just look at an existing project like firedrake and copy that. I guess this setup would be quite a bit different. The Firedrake CI builds and then pushes upon success. What we call "build" here is just running the animate regression tests which should actually be run after we've rebuilt the container i.e. the "docker" target should not depend on "build" but the other way around? The other thing I noticed is that you're referring to a docker/Dockerfile.parmmg (currently commented out) but the repo only has docker/Dockerfile.firedrake

As to when we should rebuild the container: hm not entirely sure. I think regardless of whether we have it trigger on any animate repo activity, we should do a regular rebuild (followed by running animate tests) on a fixed interval, say every week, so that if things break due to changes in the firedrake stack we still catch it if there isn't any recent activity on animate. Then additionally we could also trigger it on PRs/merges into master although maybe that doesn't actually add much (and slows down the acceptance testing) - I guess the only added benefit would be that you would catch if a test/new functionality works in this week's build but not in the very latest but I'm not sure how often that would happen and the number of merges in a week is usually fairly small anyway.

jwallwork23 commented 5 months ago

Yes that's a good point - I think we'd previously discussed making it a regular build rather than on PR. Until we figure out how to actually do the build, I'll leave it as part of the PR workflow, though.

jwallwork23 commented 5 months ago

[Apologies if you're getting emails for these commits, it must be annoying!]

Good news is I finally managed to get it to log into DockerHub and the latest workflow run is building!

jwallwork23 commented 5 months ago

Hm it was all going so well...

In this revised implementation there are now two distinct tasks: docker and test_suite. The first time, it did build successfully and the Docker image was updated. Over in #74, it was used and the error message about firedrake.pyplot has gone away. However, there seems to be an issue with the parallel setup.

I updated the jwallwork23/firedrake-parmmg PETSc branch so that it includes the most recent Firedrake-related changes. However, now when I try the Docker build again it has a mysterious MPI abort after firedrake-update finishes. (See https://github.com/pyroteus/animate/actions/runs/7700924348/job/20985967501.)

Will try re-running in case it's a fluke.

jwallwork23 commented 5 months ago

Seemed to be a one-off - weird.

Anyway, this PR is now ready for review! :)

jwallwork23 commented 5 months ago

Thanks @stephankramer, I've just pushed what I think remains to do. I can't see your other comment - was it on a commit that's been overwritten?

jwallwork23 commented 5 months ago

@stephankramer Are you happy with my final commit here to set up a cron job for regular testing? https://github.com/pyroteus/animate/pull/47/commits/2b74fcde2c9c51c035d5f2ea535cf9d04fde6b34