Key the nightly data cache on the fwl-mors dataset manifest - #37
Merged
Merged
Conversation
The Spada grid is declared in the manifest fwl-mors ships and fetched into a versioned directory, so the key digests that directory and the registry checksums fwl-io reads from the manifest, instead of a Zenodo record the fwl-mors data module no longer exposes. The restore check looks in the versioned directory and no longer looks for a leftover archive, since fwl-io places the extracted tree in one step. The tests build a manifest and show that a new record or checksum moves the key, that the Baraffe entry does not, and that a missing entry, registry or manifest stops with a diagnostic.
Checking a data root that does not exist reports it instead of creating it as a side effect of building the fetcher. The missing-registry test asserts the message it means, the comment on the sibling script matches what it does now, and the catch-all error no longer blames the manifest for failures that are not about it.
…I in the develop extra The restore check asks fwl-io whether the members it recorded when it unpacked the archive are present, and names the missing ones, instead of counting files under a directory. A data root that does not exist is reported inside the check, so calling it directly does not create the tree. The develop extra requires fwl-mors 26.9.23, which ships the dataset manifest the cache script reads, and fwl-io 26.8.31, the first release with check_dataset. The runtime dependency on fwl-mors is unchanged.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the file-count handling issue and add the required manifest error-path coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates nightly data caching to use the fwl-mors manifest and fwl-io validation.
Changes:
- Derives cache keys from the Spada dataset directory and registry checksums.
- Validates restored datasets with
check_dataset. - Updates dependencies, tests, and nightly workflow documentation.
| File | Summary | Review notes |
|---|---|---|
tools/nightly_data_cache.py |
Manifest-based cache keying and dataset validation | Moderate issue (1 vote): structural faults can produce an incorrect present-file count. |
tests/test_nightly_data_cache.py |
Tests for manifest keys and restored trees | Nit (2 votes): add a boundary or malformed-manifest case. |
pyproject.toml |
Adds required development dependency floors | No final comments. |
.github/workflows/nightly.yml |
Documents the updated cache-key flow | No final comments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ries The key material test now also checks that a re-pinned Spada record moves the version directory to that record, and that a second registry line or a DOI that is not a Zenodo record raises ResolutionError without creating a data root.
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 stopped at "Resolve the data cache key" (https://github.com/FormingWorlds/ZEPHYRUS/actions/runs/35973497916):
tools/nightly_data_cache.pyread the Spada pin frommors.data.get_zenodo_record(), which fwl-mors 26.9.23 removed when it moved its dataset pins intomors_manifest.toml.checkuses fwl-io'scheck_dataset, which names each missing file from the fetch record, instead of counting files. It is read-only and fails with a clear message on a missing or incomplete tree.developextra requiresfwl-io >= 26.8.31andfwl-mors >= 26.9.23, which the tool needs; the runtimefwl-morsfloor is unchanged.Validation of changes
checkon a real copy of the Spada grid exits 0 (1602 files present) and names a deleted file (exit 1); a missing data root exits 1 and is not created.tests/test_nightly_data_cache.py: 11 passed (macOS, Python 3.12).Checklist