Merge master into dev (test-data cache redesign) - #841
Conversation
# Conflicts: # .github/actions/load-shared-vars/action.yml # .github/actions/mamba-install-dascore/action.yml # .github/scripts/cache_test_data.py # .github/scripts/export_test_data_cache_env.py # .github/workflows/build_deploy_stable_docs.yaml # .github/workflows/run_min_dep_tests.yml # .github/workflows/runtests.yml # dascore/utils/downloader.py # docs/tutorial/coords.qmd # tests/test_utils/test_downloader.py
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe pull request centralizes CI test-data caching, replaces shared environment variables with step outputs, pins GitHub Actions, adds workflow auditing, updates registry handling, switches versioning to CI cache and workflow integration
Registry and test-data handling
Build and documentation updates
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/tutorial/coords.qmd (1)
53-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore coverage for
CoordStringandCoordSegmented.
CoordStringandCoordSegmentedare implemented coordinat types, butdocs/tutorial/coords.qmddoes not document them. Add tutorial examples, or remove the classes if these coordinates are no longer part of the public API.🤖 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 `@docs/tutorial/coords.qmd` at line 53, Update the “Update” section in docs/tutorial/coords.qmd to document the public CoordString and CoordSegmented coordinate types with tutorial examples; only remove those classes instead if they are intentionally no longer part of the public API.Source: MCP tools
🧹 Nitpick comments (1)
tests/test_utils/test_downloader.py (1)
33-33: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd positive coverage for complete registry retrieval.
The deleted test covered only the old exclusion path. Add a test that compares
get_registry_df()["name"]with every non-comment entry indascore/data_registry.txt. This protects the new contract from a future filter regression.Proposed test
+ def test_dataframe_contains_all_registry_entries(self, registry_df): + expected = [ + line.split(maxsplit=1)[0] + for line in REGISTRY_PATH.read_text().splitlines() + if line and not line.startswith("#") + ] + assert registry_df["name"].tolist() == expected🤖 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 `@tests/test_utils/test_downloader.py` at line 33, Add a positive test in the downloader test module that reads every non-comment entry from dascore/data_registry.txt and asserts it exactly matches get_registry_df()["name"]. Preserve the registry ordering and exclude only comment lines when constructing the expected entries, ensuring future filtering regressions are detected.
🤖 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.
Inline comments:
In `@docs/tutorial/coords.qmd`:
- Line 72: Update the comment describing the update step to use “shortening end
time” instead of “shorting end time,” while preserving the rest of the wording.
- Around line 123-144: Update the `snap()` tutorial example to use a fixed,
explicitly unsorted coordinate instead of `np.random.rand(10)`. Assert that the
input is unsorted and that the snapped coordinate is sorted and evenly sampled,
while retaining the min/max preservation assertions.
- Line 11: Update the first link in the tutorial paragraph to display the exact
API name BaseCoord, matching its dascore.core.coords.BaseCoord target and later
references. Before merging, run the API-documentation link validation to ensure
.cross_ref.json contains this target.
---
Outside diff comments:
In `@docs/tutorial/coords.qmd`:
- Line 53: Update the “Update” section in docs/tutorial/coords.qmd to document
the public CoordString and CoordSegmented coordinate types with tutorial
examples; only remove those classes instead if they are intentionally no longer
part of the public API.
---
Nitpick comments:
In `@tests/test_utils/test_downloader.py`:
- Line 33: Add a positive test in the downloader test module that reads every
non-comment entry from dascore/data_registry.txt and asserts it exactly matches
get_registry_df()["name"]. Preserve the registry ordering and exclude only
comment lines when constructing the expected entries, ensuring future filtering
regressions are detected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6eedcc40-9d5f-49d9-b251-c1abc4fc946d
📒 Files selected for processing (29)
.github/actions/load-shared-vars/action.yml.github/actions/mamba-install-dascore/action.yml.github/actions/prep_doc_build/action.yml.github/actions/prime-test-data-cache/action.yml.github/dependabot.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/lint.yml.github/workflows/profile.yml.github/workflows/run_min_dep_tests.yml.github/workflows/runtests.yml.github/workflows/test_doc_build.yml.github/workflows/test_free_threaded.yml.github/workflows/test_wasm.yml.github/workflows/upload_pypi.yml.gitignore.pre-commit-config.yamlbenchmarks/test_io_benchmarks.pydascore/data_registry.txtdascore/utils/downloader.pydocs/contributing/adding_test_data.qmddocs/tutorial/coords.qmdpyproject.tomltests/test_io/test_common_io.pytests/test_utils/test_downloader.py
💤 Files with no reviewable changes (3)
- dascore/data_registry.txt
- .github/scripts/export_test_data_cache_env.py
- .github/scripts/cache_test_data.py
![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: 65905697fc
ℹ️ 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".
| ::: | ||
|
|
||
| In order to manage coordinate labels and array manipulations, DASCore implements two classes, [`BaseCoord`](`dascore.core.coords.BaseCoord`), which has several associated subclasses corresponding to different types of coordinates, and [CoordManager](`dascore.core.coordmanager.CoordManager`) which manages a group of coordinates. Much like the [`Patch`](`dascore.core.patch.Patch`), instances of both of these classes are immutable (to the extent possible), so they cannot be modified in place but have methods which return new instances. | ||
| In order to manage coordinate labels and array manipulations, DASCore implements two classes, [BaseCoordinate](`dascore.core.coords.BaseCoord`), which has several associated subclasses corresponding to different types of coordinates, and [CoordManager](`dascore.core.coordmanager.CoordManager`) which manages a group of coordinates. Much like the [`Patch`](`dascore.core.patch.Patch`), instances of both of these classes are immutable (to the extent possible), so they cannot be modified in place but have methods which return new instances. |
There was a problem hiding this comment.
When this lands on dev, the conflict resolution replaces the current tutorial with older master content: this line renames the actual BaseCoord class to the nonexistent BaseCoordinate, while later hunks delete the usage and limitation sections for the still-implemented CoordSegmented and string-coordinate APIs. Preserve the parent tutorial sections so the documentation remains consistent with dascore/core/coords.py and its existing tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7c4f373 — restored the dev tutorial (string + segmented coordinate sections, BaseCoord name) and re-applied the two master-side fixes it lacked. Also purged the stale rerere recording that caused this.
coderabbitai
Bot
added
the
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #841 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17948 17950 +2
=========================================
+ Hits 17948 17950 +2
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:
|
|
✅ Documentation built: |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
A stale rerere recording resolved the coords.qmd conflict to older master content, dropping the string and segmented coordinate sections and the corrected class name. Restore the dev version and re-apply the two master-side fixes it lacked (print target and the drop-coordinate example). The recorded resolution has been forgotten so it cannot reapply.
|
Addressed the review round in 7c4f373: the coords.qmd conflict had been auto-resolved from a stale rerere recording to older master content — restored the dev tutorial (covers CoordString/CoordSegmented per the out-of-diff comment) plus the two master-side fixes, and added the suggested positive registry-coverage test. |
1 participant
Description
Routine master → dev sync bringing over the test-data cache redesign (#839) plus the zizmor hardening (#835), action SHA pins (#837), and stable-docs pre-release fixes (#825).
Non-trivial conflict resolutions, since dev's CI had diverged:
runtests.yml,run_min_dep_tests.yml): kept dev's structure (OS matrix from shared vars, tmate debug flow, qmd doc tests, network-tests job, fail-fast: false) and applied master's cache changes on top — prime-test-data-cache step in the existingsetupjobs, workflow-levelDFS_DATA_DIR, no morecache-numberplumbing, registry path filters, SHA-pinned actions (including dev's previously unpinned tmate/setup-python/setup-node).mamba-install-dascore: kept dev's weekly environment-cache key andpython-gilpin; replaced the old export/restore/prime/save block with the shared prime action (dev's restore-keys prefix-reuse idea is superseded — a cold prime is now one ~20 s clone).downloader.py: kept dev's config-driven refactor (get_config().downloader_cache_dir, fetcher proxy, retry support) and dropped itsLARGE_REGISTRY_FILES/exclude_largemachinery, which existed only to keep the now-deletedwhale_1.hdf5out of fixtures and priming; call sites intest_common_io.pyandbenchmarks/test_io_benchmarks.pyupdated. The config-override test now clearsDFS_DATA_DIRfirst, since pooch prefers that env var over the configured path (and CI sets it globally).test_free_threaded.ymlandtest_wasm.ymlused the deleted priming scripts; both now use the shared prime-test-data-cache action (the WASM suite previously seeded viaXDG_CACHE_HOMEand a private cache key — it now shares the one cross-OS cache).persist-credentialslines both branches had added independently, and applied zizmor fixes to the dev-only workflows it had never audited.Please merge with a merge commit (not squash) to preserve the sync history.
Changelog
none
Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores