Stop re-downloading cached FWL_DATA and AGNI data on every CI run - #794
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #794 +/- ##
==========================================
+ Coverage 92.63% 92.66% +0.02%
==========================================
Files 111 111
Lines 15892 15892
Branches 2847 2847
==========================================
+ Hits 14722 14726 +4
+ Misses 1170 1165 -5
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for this, @egpbos. Could you clarify what happens when the AGNI data are updated? If the AGNI That is: what happens when this record is updated? https://zenodo.org/records/21390786 |
Confirmed on CI (both runners)Ran this branch twice: a seeding run that reseeds the cache once, then a warm re-run that is the real test.
The warm re-run passed all checks (green), so the tests still pass on the warm-cache path, not just the setup step. What the two runs show
Net: fast PR checks are back to roughly the pre-June baseline, saving about 10 min per run on Linux and 6.5 min per run on macOS, on every PR. |
Good question. It doesn't trigger on that. The cache action right now only looks at two files: .github/data-manifest.yaml and src/proteus/data/proteus_manifest.toml. On main it only looks at .github/data-manifest.yaml, but it looks like that one is not kept up to date, which is the reason the cache was no longer being updated. I think indeed if all data starts using the fwl-io manifest then the CI cache should always be up to date. But I must admit that I'm not sure whether that is the plan (and even if it is, whether it will happen soon). Is it necessary for the fast PR tests to have the latest data? Or the further reaching question: is (all) data necessary at all for fast PR tests? |
No, it isn't. If we are skipping a lot of the physics for the fast PR tests then we likely don't need all of the data.
So in that case, perhaps a lot of the problems highlighted here will be resolved/fixed when fwl-io is fully implemented across all modules? Do we want to address this issue now, then, or wait a while? |
Well, the first part of this PR (particularly this line, I should have made this two commits, sorry for that) makes sure the manifest triggers cache-refresh, so I think we can address that part now already for sure. The second part (preventing AGNI redownload if the cache is fine) we could leave out for now, although it does save 200+ seconds every CI run if we do it now. |
The cache key hashed only .github/data-manifest.yaml, which has not changed since the exoplanet and mass-radius reference data were migrated to fwl-io. With a constant key, actions/cache exact-hits and never re-saves, so the fwl-io files (58 small reference files, ~190 s of serial downloads) were absent from the frozen tree, re-downloaded on every run, and discarded at job end. Hash src/proteus/data/proteus_manifest.toml (the real fwl-io pins) into the key as well, so a Zenodo re-pin busts the cache and forces a fresh save. The restore-keys prefix lets the resulting save build on the previous tree rather than re-downloading everything. This edit also changes the key once now, which seeds a complete cache on the next run.
The fetch ran unconditionally, re-pulling ~200 s of data (dominated by the thermodynamics archive) even when the warm AGNI cache already held a complete res/ tree. Guard it behind the step's existing res_complete check: skip when every required artifact is present and non-empty, and fall through to the retry/repair loop otherwise, so the repair path the unconditional fetch was added for is preserved.
b2cb4f7 to
d7a4ed9
Compare
|
Discussed this offline with @timlichtenberg. The conclusion we reached on the AGNI commit is that it's probably best to merge it like it is now. Win: speeds up CI now by 3-4 minutes, quite noticeable. Costs: a bit of maintenance (just remember to remove it after AGNI data has switched to fwl-io) and it will not automatically update to new Zenodo releases. Mitigation for that: delete the cache manually, that will always trigger a new download. Do you think this is workable @nichollsh? Also this part would only be until AGNI data has switched to fwl-io. Btw, also good to know: fwl-io development is currently blocked because Tim is waiting for admin rights to make our Dataverse mirrors public (they are private by default it seems). This may take until after the summer, hard to say. |
|
That sounds good to me! I think if we can already benefit from the changes you've made now, then that's great. We can make further developments in the future and they're all iterative/positive. It's still not entirely clear to me how AGNI/Obliqua can best utilise fwl-io. We should discuss this in more detail, since I'd like to get it right. |
What I understand is that @timlichtenberg will get around to this at some point, he's including things into fwl-io step by step. I think the plan is to just figure out step by step how to do it? Your comment nichollsh/AGNI#185 (comment) will probably be addressed that way, but maybe good if we put it on the Interra board to make sure we discuss it briefly next week? I still need an approval on this PR before merging, if you or @timlichtenberg have the time :) |
nichollsh
left a comment
There was a problem hiding this comment.
Apologies for the delay in reviewing this. Thanks for identifying this performance issue @egpbos. This PR seems to take a good step toward addressing the problem, and we will just need to manually delete the cache if the AGNI reference data update. This shouldn't be too often/numerous before we get around to the complete fwl-io implementation.
I am good with this being merged as-is, and we can continue to iterate on the CI performance (particularly relating to the AGNI data) in future PRs.
Description
Two cheap caching fixes for the fast PR checks, addressing part of #793. Both target the "Set up PROTEUS environment" step, which is where nearly all of the recent slowdown lives (Linux setup grew from ~4.5 min in May to ~14 min now; the unit tests themselves are still ~90 s). Both the Linux and macOS legs were affected.
FWL_DATA cache (solution 1 in #793). The cache key hashed only
.github/data-manifest.yaml, which has not changed since the exoplanet and mass-radius reference data were moved to fwl-io (#782). With a constant key,actions/cacheexact-hits and never re-saves, so the 58 fwl-io reference files (~190 s of serial per-file downloads) were absent from the frozen tree, re-downloaded on every run, and thrown away at job end. This PR hashessrc/proteus/data/proteus_manifest.toml(the real fwl-io pins) into the key as well, so:restore-keysprefix lets that save build on the previous tree rather than re-downloading everything.AGNI spectral-data fetch (solution 2 in #793). The fetch ran unconditionally, re-pulling ~200 s of data (dominated by the thermodynamics archive) even when the warm AGNI cache already held a complete
res/tree. It is now guarded by the step's existingres_completecheck: skip when every required artifact is present and non-empty, and fall through to the existing retry/repair loop otherwise. The repair path the unconditional fetch was added for (a partial or zero-byteres/from a green-but-incomplete download) is preserved, because such a tree failsres_completeand still triggers the fetch.Solutions 3 (skip the real-binary toolchain for the mocked unit job) and 4 (decouple downloads from the OS matrix) from #793 are deliberately out of scope here; they need verification and design first.
Validation of changes
Confirmed on CI (both runners): a warm re-run downloads 0 reference files and skips the AGNI fetch, cutting the job from 16m30s to 6m39s on Linux and 11m50s to 5m19s on macOS. Full evidence and the two-run explanation are in this comment.
Test configuration: CI-config only (no Python touched); the two guarding unit tests (
test_ci_setup_installs_every_declared_extra,test_ci_config_never_pins_user) pass locally.Checklist