CI: test cache, bump tested version, global config - #591
Conversation
|
Warning Rate limit exceeded@d-chambers has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds two composite GitHub Actions (load shared variables and cache test data), introduces a centralized Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 #591 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 129 129
Lines 11274 11274
=======================================
Hits 11266 11266
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/get_coverage.yml (1)
18-22: LGTM: Consistent Python version management.The workflow adopts the shared Python version approach. Consider adding a blank line after the load-shared-vars step (line 18) for consistency with other workflows.
.github/actions/load-shared-vars/action.yml (1)
34-39: Consider removing unused matrix reading code.Lines 36-39 read
test_matrixandmin_deps_matrixfrom the config file and print them, but:
- These keys are not defined in
.github/shared-vars.yml- The comment on lines 34-35 states that workflows need to read the file directly for matrix strategy
- The values are only printed to stdout and not exposed as outputs or environment variables
This code appears to be dead code that serves no functional purpose.
🔎 Proposed cleanup
# Write to GITHUB_ENV with open(os.environ['GITHUB_ENV'], 'a') as env_file: for key, value in env_vars.items(): env_file.write(f"{key}={value}\n") print(f"✅ Set {key}={value}") - - # For matrix values, we can't set them directly in env - # Workflows will need to read the file directly for matrix strategy - test_matrix = python_config.get('test_matrix', []) - min_deps_matrix = python_config.get('min_deps_matrix', []) - print(f"📋 Test matrix available: {test_matrix}") - print(f"📋 Min deps matrix available: {min_deps_matrix}") EOFAlternatively, if matrix configurations will be added to
shared-vars.ymlin the future, ensure they are defined and document how workflows should consume them.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/actions/cache-test-data/action.yml.github/actions/load-shared-vars/action.yml.github/actions/mamba-install-dascore/action.yml.github/shared-vars.yml.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/upload_pypi.yml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/test_doc_build.yml
21-21: could not parse action metadata in "/home/jailuser/git/.github/actions/mamba-install-dascore": line 4: unexpected key "type" for definition of input "install-package"
(action)
.github/workflows/get_coverage.yml
20-20: could not parse action metadata in "/home/jailuser/git/.github/actions/mamba-install-dascore": line 4: unexpected key "type" for definition of input "install-package"
(action)
.github/workflows/build_deploy_master_docs.yaml
42-42: could not parse action metadata in "/home/jailuser/git/.github/actions/mamba-install-dascore": line 4: unexpected key "type" for definition of input "install-package"
(action)
.github/workflows/build_deploy_stable_docs.yaml
36-36: could not parse action metadata in "/home/jailuser/git/.github/actions/mamba-install-dascore": line 4: unexpected key "type" for definition of input "install-package"
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.14)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: Run benchmarks
🔇 Additional comments (14)
.github/actions/cache-test-data/action.yml (3)
1-25: LGTM: Well-structured prerequisites verification.The action metadata is clear, and the prerequisites check with helpful error messages will aid debugging if Python or Pooch is missing.
37-45: LGTM: Solid caching strategy.The cache key combines content hash with a manual invalidation mechanism, and
enableCrossOsArchiveenables efficient cache sharing across platforms.
27-35: No issues found with Pooch API usage.The methods
pooch.file_hash()andpooch.os_cache()are both valid and available in Pooch v1.2+, which is the minimum version required by dascore. The GitHub Actions workflow code is correct as written.Likely an incorrect or invalid review comment.
.github/workflows/build_deploy_stable_docs.yaml (1)
34-38: LGTM: Consistent with master docs workflow.The changes mirror those in
build_deploy_master_docs.yaml. Ensure the load-shared-vars action exists and properly setsPYTHON_DEFAULT(same verification as previous workflow applies)..github/workflows/upload_pypi.yml (1)
17-26: LGTM: Centralized Python version for package publishing.The workflow now uses the shared Python version. For package building and publishing, the specific Python version matters less, but consistency is good.
.github/workflows/lint.yml (1)
18-25: LGTM: Centralized Python version for linting.The workflow now uses the shared default Python version. This ensures consistency across all CI workflows.
.github/workflows/run_min_dep_tests.yml (1)
34-34: Remove Python 3.14 consideration from review—already stable and fully supported.Python 3.14 was released on October 7, 2025 as a stable release, not beta or RC. The GitHub Actions setup-python action includes Python 3.14 now, and Python 3.14.2 is available as of December 2025. No verification is needed; the test matrix is valid.
.github/workflows/build_deploy_master_docs.yaml (2)
40-40: The load-shared-vars action is properly configured.The action exists at
.github/actions/load-shared-vars/action.ymland correctly exportsPYTHON_DEFAULT(set to 3.13) along withPYTHON_LATESTandPYTHON_RELEASEto the environment viaGITHUB_ENV. The underlying.github/shared-vars.ymlconfiguration file is in place.
40-44: Fix actionlint error in mamba-install-dascore action metadata.The
typekey is not supported for inputs in action metadata. Theinstall-packageinput in.github/actions/mamba-install-dascore/action.ymlhas an invalidtype: booleankey that must be removed. Additionally, thedefaultvalue should be a string ("true"), not a boolean..github/workflows/runtests.yml (1)
36-36: Python 3.14 is now available and testable across all platforms.The GitHub Actions setup-python action includes Python 3.14, and Python 3.14.2 is available as of December 8, 2025. Conda-forge has Python 3.14 available and provides a wide selection of packages for use with it. The mamba-install-dascore action will successfully provision Python 3.14 since environment.yml contains no Python version constraints, and the Python 3.14 migration on conda-forge is 77% complete with 1273 packages rebuilt for Python 3.14.
However, update pyproject.toml to include Python 3.14 in the classifiers list (currently only declares support through 3.13).
.github/actions/mamba-install-dascore/action.yml (1)
73-75: LGTM: Clean delegation to dedicated cache action.Extracting cache logic into a dedicated composite action improves maintainability and separation of concerns.
.github/workflows/profile.yml (2)
21-25: LGTM: Proper integration of shared variables.The workflow correctly loads shared variables before referencing
env.PYTHON_DEFAULT, establishing the proper dependency chain.
30-33: LGTM: Test data caching before benchmarks.Adding explicit test data caching before the benchmark run aligns with the PR objective of making test data cache more robust.
.github/workflows/test_doc_build.yml (1)
19-23: LGTM: Consistent shared variables pattern.The workflow correctly loads shared variables before using
env.PYTHON_DEFAULT, following the same pattern as other updated workflows.Note: The static analysis hint mentions an issue with the "type" key in the mamba-install-dascore action, but this relates to the action's metadata file (line 22 in
action.yml), not this workflow file. The issue is that GitHub Actions doesn't support thetype: booleansyntax shown in line 22 of.github/actions/mamba-install-dascore/action.yml. If this causes problems, thetype: booleandeclaration should be removed from that action's input definition.
CodSpeed Performance ReportMerging #591 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/run_min_dep_tests.yml (1)
38-47: Consider movingifcondition to the setup job to avoid wasted runs.The
ifcondition on line 47 only applies totest_code_min_deps, meaning thesetupjob will still run when theno_cilabel is present. While minor, you could add the same condition to the setup job to avoid unnecessary checkout and variable loading.🔎 Proposed fix
setup: runs-on: ubuntu-latest + if: github.event_name == 'push' || !contains(github.event.pull_request.labels.*.name, 'no_ci') outputs: python-matrix: ${{ steps.load-vars.outputs.min-deps-matrix }}.github/workflows/runtests.yml (1)
40-46: Same consideration forifcondition placement.As with
run_min_dep_tests.yml, the setup job runs unconditionally. Consider adding theifcondition to avoid wasted runs whenno_cilabel is present..github/actions/load-shared-vars/action.yml (2)
39-41: Consider adding error handling for file loading.If
.github/shared-vars.ymldoesn't exist or contains invalid YAML, the action will fail with an unclear error. Adding try-except with a descriptive error message would improve debugging.🔎 Proposed fix
- # Load shared variables file - with open('.github/shared-vars.yml', 'r') as f: - config = yaml.safe_load(f) + # Load shared variables file + try: + with open('.github/shared-vars.yml', 'r') as f: + config = yaml.safe_load(f) + except FileNotFoundError: + print("❌ Error: .github/shared-vars.yml not found") + raise SystemExit(1) + except yaml.YAMLError as e: + print(f"❌ Error parsing shared-vars.yml: {e}") + raise SystemExit(1)
53-60: Handle missing keys defensively.If
pythonkey or nested matrix keys are missing from the config, the action will output empty arrays[]. This is safe but might cause downstream workflow failures that are hard to debug. Consider logging a warning.🔎 Proposed fix
# Also output specific matrices for job outputs python_config = config.get('python', {}) + if not python_config: + print("⚠️ Warning: 'python' section not found in shared-vars.yml") test_matrix = python_config.get('test_matrix', []) + if not test_matrix: + print("⚠️ Warning: 'python.test_matrix' not found in shared-vars.yml") min_deps_matrix = python_config.get('min_deps_matrix', []) + if not min_deps_matrix: + print("⚠️ Warning: 'python.min_deps_matrix' not found in shared-vars.yml")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/actions/load-shared-vars/action.yml.github/doc_environment.yml.github/shared-vars.yml.github/workflows/run_min_dep_tests.yml.github/workflows/runtests.yml
💤 Files with no reviewable changes (1)
- .github/doc_environment.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/shared-vars.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.14)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.14)
- GitHub Check: test_code_min_deps (macos-latest, 3.14)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.14)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.14)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.14)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: Run benchmarks
- GitHub Check: test_build_docs
🔇 Additional comments (5)
.github/workflows/run_min_dep_tests.yml (1)
26-34: Setup job correctly loads shared variables.The setup job properly checks out the repository and loads shared variables using the composite action, exposing the
min-deps-matrixoutput for downstream jobs..github/workflows/runtests.yml (2)
28-36: Setup job correctly loads test matrix.The setup job properly loads the
test-matrixoutput from shared variables, enabling dynamic Python version configuration for the test matrix.
53-58: Minor quote style changes look fine.These are cosmetic changes (double to single quotes) with no functional impact.
.github/actions/load-shared-vars/action.yml (2)
1-10: Clean action interface with clear outputs.The action definition provides well-documented outputs for both test and min-deps matrices.
25-37: The flatten_dict implementation is correct and handles edge cases well.The recursive flattening correctly handles nested dicts, lists (as JSON), and primitive values with proper uppercase key transformation.
|
✅ Documentation built: |
1 participant
Description
This PR makes the test data cache between jobs more robust , bumps the tested versions to include python 3.14, and creates a global config file for CI variables.
Checklist
I have (if applicable):
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.