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

Cache VTK build in ASTE CI #211

Merged
merged 8 commits into from
Oct 28, 2024
Merged

Cache VTK build in ASTE CI #211

merged 8 commits into from
Oct 28, 2024

Conversation

fsimonis
Copy link
Member

@fsimonis fsimonis commented Oct 25, 2024

Main changes of this PR

This PR uses the new ASTE CI image https://hub.docker.com/r/precice/ci-aste which is based on precice/precice:nightly and installs a working version of VTK from source.

https://github.com/precice/ci-images/blob/master/ci-aste.dockerfile

TODO

  • keep or clean ci-image

Author's checklist

  • I used the pre-commit hook and used pre-commit run --all to apply all available hooks.
  • I added a test to cover the proposed changes in our test suite.
  • I updated the documentation in docs/README.md.
  • I added a changelog entry in ./changelog-entries/ (create if necessary).
  • I updated potential breaking changes in the tutorial precice/tutorials/aste-turbine.

@fsimonis fsimonis requested a review from davidscn October 25, 2024 15:23
@davidscn
Copy link
Member

I am not in favor of merging this change:

In the current CI, we simply build VTK inside the preCICE nightly image. This is at the moment only a temporary workaround for the broken ubuntu packages and will most likely be resolved with a future ubuntu version (24.10 is at least known to work).

Certainly, building VTK in each PR takes time (about 45mins or so). However, merging this or not is mostly a question of how much development is at the moment being pursued: with these changes, we would build VTK every day and push an image. New PRs are at the moment far below (maybe two per month).

Merging here is wasteful in my opinion. What we could consider is triggering an image update not every night, but by a PR or so.

@fsimonis
Copy link
Member Author

Merging here is wasteful in my opinion. What we could consider is triggering an image update not every night, but by a PR or so.

It is wasteful for sure.

Given that the base image doesn't change, we could maybe get away with caching the installed VTK files using a long TTL. At best, this can be done whilst installing to /usr/local/. Otherwise, we may need to install to /opt.

@davidscn
Copy link
Member

Given that the base image doesn't change, we could maybe get away with caching the installed VTK files using a long TTL. At best, this can be done whilst installing to /usr/local/. Otherwise, we may need to install to /opt.

I don't know how such a caching would look like. We could also store the VTK install in a separate image and the use docker compose. The result should be somewhat similar to a caching mechanism.

@fsimonis
Copy link
Member Author

According to the GitHub docs

Workflow runs can restore caches created in either the current branch or the default branch (usually main).

Meaning we can keep a cache alive on develop and use it in PRs. I'll try this here

@fsimonis fsimonis changed the title Use ASTE ci-image Cache VTK build in ASTE CI Oct 28, 2024
@davidscn
Copy link
Member

I think that's now pretty much the ideal solution. If you push now a dummy commit, the VTK build show not be triggered, correct?

@fsimonis
Copy link
Member Author

Triggered and reused the cache.

@fsimonis fsimonis self-assigned this Oct 28, 2024
@fsimonis
Copy link
Member Author

Ok I'll squash and merge this to make debugging the other PRs quicker.

@fsimonis fsimonis merged commit 8cc1890 into develop Oct 28, 2024
6 checks passed
@fsimonis fsimonis deleted the use-ci-image branch October 28, 2024 15:08
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