Prime CI test data from one cross-OS clone of DASDAE/test_data - #839
Conversation
Replace the per-OS pooch download loop with a single depth-1 checkout of the test_data repo, copied into pooch's cache layout with sha256 verification and shared across all operating systems via one enableCrossOsArchive cache entry keyed on the registry hash. The matrix workflows' setup jobs ensure the cache exists (lookup-only) before the test jobs start, so at most one job builds it; the mamba install action keeps an inline self-heal prime for single-job workflows and evicted caches. Remove whale_1.hdf5 (817 MB, plain-HTTP university host, always skipped by tests) from the registry and require all test-suite data to live in DASDAE/test_data.
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesThe PR consolidates test-data caching into a reusable GitHub Actions composite action and a registry-driven Python script. Workflows use Test data cache
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #839 +/- ##
==========================================
- Coverage 99.93% 99.93% -0.01%
==========================================
Files 145 145
Lines 12927 12913 -14
==========================================
- Hits 12919 12905 -14
Misses 8 8
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:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b9749e67e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "dascore/data_registry.txt not found; checkout dascore first." >&2 | ||
| exit 1 | ||
| fi | ||
| echo "key=test-data-${REGISTRY_HASH}-${INPUT_CACHE_NUMBER}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
When DATA_VERSION is bumped without a registry edit—the cache-reset case documented in dascore/constants.py—this key remains unchanged, so the restore reports a hit and skips priming even though the cached files exist only under the old .test_data_cache/<DATA_VERSION> directory. Pooch will then lazily download every registry file again in each CI job, and the immutable stale cache will keep causing misses until every caller manually changes cache-number; include DATA_VERSION (or a hash of the relevant constant) in the key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — DATA_VERSION is now parsed into the cache key (test-data---), so a version bump invalidates automatically.
| # jobs start, so at most one job builds it. On a simultaneous cold miss | ||
| # the other test workflow's setup job may benignly race-prime the same | ||
| # key; the losing save just logs a warning. | ||
| - uses: ./.github/actions/prime-test-data-cache |
There was a problem hiding this comment.
When a pull request changes only dascore/data_registry.txt, this newly added priming step never runs because the pull_request.paths whitelist in this workflow includes Python, workflow, and action files but not the registry; the minimum-dependency workflow has the same omission. The always-running lint workflow does not execute the prime script or unit tests, so a missing source file, incorrect hash, or disallowed URL can merge and only fail after the subsequent push to master or dev; add the registry path to the test workflow triggers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added dascore/data_registry.txt to both test workflows' path filters in 66512a5.
Drop the cache-number plumbing through workflows and the mamba install action; the single reset knob now lives in prime-test-data-cache. Parse DATA_VERSION into the cache key so a version bump invalidates the cache instead of leaving a stale-but-hit entry.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/mamba-install-dascore/action.yml (1)
23-26: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet
DFS_DATA_DIRwheremamba-install-dascoreprepares test-data caches.The workflow-level
DFS_DATA_DIRdeclarations are not in job scope, so theprepare-test-data: "true"callers needenv: DFS_DATA_DIR: ${{ github.workspace }}/.test_data_cacheon either the job or themamba-install-dascorestep to avoid the restored cache being ignored by pooch. The remaining callers already do not passcache-number.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/mamba-install-dascore/action.yml around lines 23 - 26, Update the mamba-install-dascore action’s prepare-test-data path so callers enabling prepare-test-data receive DFS_DATA_DIR set to ${{ github.workspace }}/.test_data_cache at job or action-step scope. Ensure this environment variable is present when restoring, priming, and saving test-data caches, while preserving existing cache-number behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/actions/mamba-install-dascore/action.yml:
- Around line 23-26: Update the mamba-install-dascore action’s prepare-test-data
path so callers enabling prepare-test-data receive DFS_DATA_DIR set to ${{
github.workspace }}/.test_data_cache at job or action-step scope. Ensure this
environment variable is present when restoring, priming, and saving test-data
caches, while preserving existing cache-number behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f626a0c8-4b49-410f-b64f-ee6b5d1e4fa6
📒 Files selected for processing (18)
.github/actions/mamba-install-dascore/action.yml.github/actions/prime-test-data-cache/action.yml.github/scripts/cache_test_data.py.github/scripts/export_test_data_cache_env.py.github/scripts/prime_test_data.py.github/workflows/build_deploy_master_docs.yaml.github/workflows/build_deploy_stable_docs.yaml.github/workflows/get_coverage.yml.github/workflows/profile.yml.github/workflows/run_min_dep_tests.yml.github/workflows/runtests.yml.github/workflows/test_doc_build.yml.gitignoredascore/data_registry.txtdascore/utils/downloader.pydocs/contributing/adding_test_data.qmdtests/test_io/test_common_io.pytests/test_utils/test_downloader.py
💤 Files with no reviewable changes (4)
- dascore/data_registry.txt
- dascore/utils/downloader.py
- .github/scripts/export_test_data_cache_env.py
- .github/scripts/cache_test_data.py
| Of course, not all data can easily be generated in python. For example, testing [support for new file formats](./new_format.qmd) typically requires a test file. | ||
|
|
||
| If you have a small file that isn't already hosted on a permanent site, you can put it into [dasdae's data repo](https://github.com/DASDAE/test_data). Simply clone the repo, add you file format, and push back to master or open a PR on a separate branch and someone will merge it. | ||
| All test-suite data files must be hosted in [dasdae's data repo](https://github.com/DASDAE/test_data); the registry only accepts files from that repo (enforced by a unit test). Simply clone the repo, add your file, and push back to master or open a PR on a separate branch and someone will merge it. Files for other purposes (e.g. documentation examples) may be hosted elsewhere later, but they don't go in the data registry. |
There was a problem hiding this comment.
They can go in the data_registry; just not the test_data repo.
There was a problem hiding this comment.
Reworded in 66512a5: externally hosted files can be listed in the registry; CI just doesn't cache them, so tests shouldn't depend on them.
| def test_urls_hosted_in_test_data_repo(self, registry_df): | ||
| """ | ||
| All test-suite data files must be hosted in the DASDAE test_data repo; | ||
| CI primes its cache from a single checkout of it (see | ||
| .github/scripts/prime_test_data.py). Files hosted elsewhere would be | ||
| re-downloaded per job and bypass the shared cache. | ||
| """ | ||
| pattern = r"(?i)^https?://github\.com/dasdae/test_data/raw/master/" | ||
| bad = registry_df[~registry_df["url"].str.match(pattern)] |
There was a problem hiding this comment.
we dont need this
Per review: files for non-test-suite purposes may be listed in the registry with external hosts, so drop the unit test forbidding them (the prime script already skips them with a warning). Add data_registry.txt to the test workflows' path filters so registry-only PRs validate their entries before merge, and make the DATA_VERSION extraction tolerant of annotated or tab-separated assignments.
1 participant
Description
Reworks CI test-data caching to fix intermittent download failures and shrink cache usage from ~3.3 GB to ~280 MB.
Previously each OS primed its own cache by downloading all 47 registry files serially with no retries (~1.1 GB × 3 OS keys), and any registry edit forced a cold re-download on every OS with up to 18 matrix jobs racing to save. 46 of the 47 files already live in the public DASDAE/test_data repo, so CI now:
setupjob, gated by alookup-onlycache probe) and copies the registry files into pooch's cache layout with sha256 verification (.github/scripts/prime_test_data.py, stdlib-only),enableCrossOsArchive, key =test-data-<hashFiles(data_registry.txt)>-<cache-number>) that all 18 test jobs plus the coverage/profile/docs workflows restore,DFS_DATA_DIRenv var; if the cache is ever missing, the mamba install action self-heal-primes inline, and pooch's lazy fetch remains as a final fallback.The outlier
whale_1.hdf5(817 MB from a plain-HTTP university server — ~75 % of the old cache, yet always skipped by tests) is removed from the registry entirely. A new unit test requires all registry URLs to point at DASDAE/test_data; the prime script skips-with-warning on non-hosted rows so stable-docs builds of older tags (whose registry still contains whale) keep working.TestDataCacheInfoand the two old CI scripts are deleted.Registry↔repo drift now fails loudly at prime time (hash mismatch aborts the setup job) instead of surfacing as confusing pooch errors across the matrix.
Changelog
whale_1.hdf5example dataset is no longer in the data registry, sodascore.utils.downloader.fetch("whale_1.hdf5")raises.dascore.utils.downloader.TestDataCacheInfoandget_test_data_cache_info, which existed only to key the CI test-data cache.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Documentation
Maintenance