Key the nightly data cache on the dataset ZEPHYRUS actually resolves - #32
Merged
Merged
Conversation
The nightly cached the FWL data tree under a key that never changed. actions/cache writes an entry only when the primary key misses, so a key that always hits is never rewritten and the tree it holds cannot follow the data. That the nightly is green today is a property of the data not having moved, not of the cache working. The exposure is sharper here than for a versioned dataset. ZEPHYRUS reaches one dataset through its fwl-mors dependency, the Spada stellar-evolution grid, and that grid unpacks into the unversioned directory stellar_evolution_tracks/Spada. mors skips the download whenever that directory is present, so a re-pinned grid does not land beside the old one and announce itself: the stale tree keeps satisfying the check indefinitely, and nothing on disk can notice. The cache key is the only thing that can. Spada is also outside the fwl-io manifest, so there is no committed registry to hash. The key is instead derived from the Zenodo record that pins the deposit, read from the installed fwl-mors. It moves when the deposit moves and stays put otherwise, so a steady-state night exact-hits its own entry. There is deliberately no restore-keys prefix, which is where this differs most from the same fix in JANUS. A prefix fallback restores the previous grid into the very directory mors checks for, so on the one night the key moves, mors would skip the download, the post-restore check would be skipped too because a prefix restore reports no cache hit, and the stale tree would then be saved under the new key and cemented there. JANUS can afford the prefix because its dataset lands in a versioned directory, so a moved key leaves the new path genuinely absent. Here a moved key has to miss outright. Resolving the key refuses to run when fwl-mors is missing, no longer exposes the dataset record, or reports no record for Spada, since a key that came out empty would equal the restore-key prefix, exact-hit its own entry, and freeze the tree again with nothing failing. On an exact hit the workflow then checks that the grid is genuinely unpacked, holding a plausible file count across its per-composition subdirectories with no archive left behind by an interrupted extraction, because there is no registry here to compare against file by file. The OSF project that mirrors the deposit is not part of the digest. Its id does not change when the files behind it change, so hashing it would imply a coverage this key does not have; mirror drift is untracked and says so, as it already does for the OSF pins in JANUS. Tests pin the key to the record in both directions, pin the absence of a prefix fallback, pin the output line the cache step reads, pin the restore check against an empty tree, a short one and one still holding its archive, and assert the dataset the key tracks is the dataset the suite actually downloads.
The workflow comment and the mirror test both said the key tracks the OSF mirror. It does not. The digest carries the Zenodo record that pins the Spada grid and the directory the grid unpacks into, and nothing else. Leaving the mirror out is deliberate, because its project id does not change when the files behind it do, so hashing it would imply a coverage the key does not have. Mirror drift is untracked, and both artifacts now say so. The mirror check compared the key before and after patching the project id. That only notices a digest that reads the attribute at resolve time; one carrying the mirror id as a literal leaves the key untouched and passed. It now pins the digest material itself, which also catches a pin being dropped or the mirror folded into an existing value. The unpack directory is hashed alongside the record, but nothing checked it, so a refactor could have dropped it from the hash while leaving it in the returned map. A rename of it now has to move the key. The helper's module docstring names both pins and states why a key that never moves freezes the cached tree, since the workflow comment sends the reader there for the reasoning.
Contributor
There was a problem hiding this comment.
Pull request overview
(Read in copilot-instructions.md...)
This PR fixes ZEPHYRUS nightly cache staleness by deriving the actions/cache key from the actual Spada dataset pin resolved via the installed fwl-mors, and adds a post-restore structural check so an exact cache hit is validated before running the suite.
Changes:
- Add
tools/nightly_data_cache.pyto (1) derive a cache key from the Spada Zenodo record and (2) validate the restored grid looks unpacked and complete. - Update
.github/workflows/nightly.ymlto use the derived key (with norestore-keys) and to run the restore check on exact cache hits. - Add unit tests that pin the key behavior, output format, workflow wiring, and restore-check failure modes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tools/nightly_data_cache.py |
New helper to resolve a data-cache key from fwl-mors pins and validate a restored Spada grid tree. |
tests/test_nightly_data_cache.py |
New unit tests covering key derivation, CLI output, workflow integration, and restore validation behavior. |
.github/workflows/nightly.yml |
Switch nightly cache key from a literal to the derived key; add exact-hit restore validation step. |
Suppressed comments (2)
tools/nightly_data_cache.py:126
- This docstring mentions collapsing onto a "restore-key prefix", but the relevant behavior here is collapsing the cache key to just the
KEY_PREFIXvalue. Updating the wording keeps the error contract aligned with the workflow configuration (norestore-keys).
When the pins cannot be resolved, or the digest comes out empty and
would collapse the key onto the restore-key prefix.
tests/test_nightly_data_cache.py:111
- This docstring says a "restore prefix" would win, but the workflow intentionally has no
restore-keys. The actual failure mode is writing an empty/invalid key (effectively just the prefix), which would then hit forever and freeze the tree.
A malformed line leaves the cache key empty, the restore prefix always
wins, and the tree freezes with nothing failing.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+159
to
+167
| # Populated, but the archive was left behind by an interrupted unpack. | ||
| for i in range(mod.MIN_FILES): | ||
| comp = grid / f'X0p8{i % 20}_Z0p002_A1p875' | ||
| comp.mkdir(exist_ok=True) | ||
| (comp / f'track_{i}.dat').write_text('x', encoding='utf-8') | ||
| (base / 'fs255_grid.tar.gz').write_text('x', encoding='utf-8') | ||
| count, problems = mod.check_restored(tmp_path) | ||
| assert count == mod.MIN_FILES + 5 | ||
| assert any('left unextracted' in p for p in problems) |
Comment on lines
+42
to
+44
| Both subcommands fail with a diagnostic rather than degrade: an empty or | ||
| partial digest would collide with the workflow's restore-key prefix and | ||
| freeze the cached tree with nothing reporting it. |
Comment on lines
+131
to
+134
| raise ResolutionError( | ||
| 'no pin was resolved, so the key would be the bare restore-key prefix ' | ||
| 'and the cached tree could never be rewritten.' | ||
| ) |
Comment on lines
+80
to
+81
| # An empty digest would equal the workflow's restore-key prefix, exact-hit | ||
| # its own entry, and freeze the tree with nothing reporting it. |
On 3.10 the resolver can only reach an fwl-mors from before the Spada Zenodo record accessor existed, because every later release requires 3.11, so the cache key cannot be resolved on that interpreter at all and the tests error on the missing accessor. The skip is keyed on the interpreter version, not on the attribute being present. On 3.11 and above a missing accessor still fails the suite, which is deliberate: there it means the dependency moved underneath us, and noticing that is what this file is for. The helper itself already stops with a clear diagnostic on such an interpreter, and the nightly workflow pins Python 3.12, so nothing changes at run time.
Four artifacts explained an empty or partial digest as colliding with the workflow's restore-key prefix, a mechanism this branch removed. With no prefix configured, an empty digest leaves the key as the constant prefix alone, which exact-hits its own cache entry on every run and freezes the tree just as a fixed literal key did. The tool docstrings, the resolution error, and the test rationale now say that, and the error names the actual prefix value so a collapsed key is recognisable in a log. The key-line test also claimed a malformed output line lets a restore prefix win silently. With no prefix, the cache step rejects an empty key, so the nightly fails at that step instead; the docstring now states that. The restore check test creates its file tree against a floor patched down to 24, with every assertion expressed against the patched constant, so the count-to-floor relationship it proves is unchanged while the test stays inside the unit wall-time budget. The remaining first-test cost is the one-time mors import, which no test-level change removes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The nightly workflow caches the FWL data tree under a key that never changed,
fwl-data-nightly-1, frozen since 2026-07-10.actions/cachewrites an entry only when the primary key misses, so a key that always hits is never rewritten and the tree it holds cannot follow the data. The nightly is green today because the data has not moved, not because the cache works.The exposure is sharper here than for a versioned dataset. ZEPHYRUS reaches one dataset through its
fwl-morsdependency, the Spada stellar-evolution grid, and that grid unpacks into the unversioned directorystellar_evolution_tracks/Spada.morsskips the download whenever that directory is present, so a re-pinned grid does not land beside the old one and announce itself: the stale tree keeps satisfying the check indefinitely, and nothing on disk can notice. The cache key is the only thing that can.Spada also sits outside the fwl-io manifest, so there is no committed registry to hash. The key is instead derived from the Zenodo record that pins the deposit, read from the installed
fwl-mors, so it moves when the deposit moves and stays put otherwise. There is deliberately norestore-keysprefix, which is where this differs from the same fix in JANUS: a prefix fallback would restore the previous grid into the very directorymorschecks for, so on the one night the key moves,morswould skip the download, the post-restore check would be skipped too because a prefix restore reports no cache hit, and the stale tree would then be saved under the new key and cemented there. JANUS can afford the prefix because its dataset lands in a versioned directory, so a moved key leaves the new path genuinely absent. Here a moved key has to miss outright.Resolving the key refuses to run when
fwl-morsis missing, no longer exposes the dataset record, or reports no record for Spada, since a key that came out empty would freeze the tree again with nothing failing. On an exact hit the workflow checks that the grid is genuinely unpacked before any test runs. The OSF project that mirrors the deposit is not part of the digest, because its id does not change when the files behind it do; mirror drift is untracked and the code says so.After merging, please dispatch the ZEPHYRUS nightly manually on
mainonce. Cache writes are branch-scoped, somain's first scheduled run after this lands will still miss and fetch Spada in full, about 339 MB, before saving. There is no prefix to soften that, deliberately.Validation of changes
Two dispatched runs on the branch. Run 31219873238 logs
Cache not found for input keys:against the single derived key, with no prefix in the list, then 46 passed in 19.56 s andCache saved with key:. Run 31219990369 logs an exactCache hit for:the same key, the pre-test probe reportingstellar_evolution_tracks/Spada/fs255_grid: 1602 files present, 46 passed in 11.39 s, andnot saving cache. The cache API shows the branch-scoped entry created at the first run's save and last read at the second run's hit.The key reproduces across two different
fwl-morsinstalls, a local editable checkout and the wheel CI resolves, giving the same digest. Re-pinning the record moves the key; the same pin gives the same key across repeated calls; a missing record raises rather than degrading. The probe's file count matches a real tree on disk exactly, 1602 files and 339 MB. Mutations over the key logic and the restore check are each caught by exactly one assertion, and the suite is green when they are restored. 43 unit tests pass, ruff is clean, and the marker, quality and file-size validators pass. Tested on macOS with Python 3.12 and on the workflow's ubuntu-latest.Checklist
Relevant people
@nichollsh