Skip to content

Fix codspeed - #620

Merged
d-chambers merged 6 commits into
masterfrom
fix_codspeed
Mar 11, 2026
Merged

Fix codspeed#620
d-chambers merged 6 commits into
masterfrom
fix_codspeed

Conversation

@d-chambers

@d-chambers d-chambers commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

Description

This PR tries to fix the codspeed profiling. The problem is the new DASVader files require a newer version of hdf5 which gets installed with the micromamba env but not necessarily pip/uv.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings and/or appropriate doc page.
  • included tests. See testing guidelines.
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

Summary by CodeRabbit

  • Chores
    • Added automated priming and cross-OS caching of shared test data to CI, with conditional download on cache miss to speed workflows.
    • Introduced a dedicated cache-priming job used by tests, docs, profiling and other workflows, improving reproducibility and reducing redundant data fetches.
    • Exposed cache status and key outputs and a public mode input for flexible restore/save behavior.
    • Added a helper to populate the local test-data cache for faster priming.

@coderabbitai

coderabbitai Bot commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@d-chambers has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 45 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c126f00c-0d49-42c8-bfc0-76ba83a56ed4

📥 Commits

Reviewing files that changed from the base of the PR and between 8c4ac64 and abef7ed.

📒 Files selected for processing (5)
  • .github/actions/cache-test-data/action.yml
  • .github/actions/mamba-install-dascore/action.yml
  • .github/workflows/prime_test_data_cache.yml
  • .github/workflows/run_min_dep_tests.yml
  • .github/workflows/runtests.yml

Walkthrough

Adds CI test-data priming: a new priming script, refactors the cache-test-data composite action to compute OS-scoped DATA_CACHE_KEY/DATA_CACHE_PATH with mode/outputs, and integrates a prime_test_data_cache job into multiple workflows so caches are restored/primed before tests and benchmarks. (50 words)

Changes

Cohort / File(s) Summary
Cache Action
.github/actions/cache-test-data/action.yml
Reworked composite action: added mode input and cache-hit/cache-key outputs; computes DATA_CACHE_KEY (OS-scoped) and DATA_CACHE_PATH; split logic into get-key, restore, and save steps; restore-keys include runner.os.
Cache Priming Script
.github/scripts/cache_test_data.py
New script that loads the DASCore test-data registry and fetches each registered file to prime the local pooch cache; prints progress and completes.
Workflows — add prime_test_data_cache & integrate cache
.github/workflows/get_coverage.yml, .github/workflows/prime_test_data_cache.yml, .github/workflows/profile.yml, .github/workflows/run_min_dep_tests.yml, .github/workflows/runtests.yml, .github/workflows/test_doc_build.yml
Added prime_test_data_cache job (matrix: ubuntu/macos/windows) and integrated it into existing workflows via needs; each workflow restores the test-data cache, conditionally runs the priming script on cache miss, and conditionally saves the primed cache; some triggers extended to include .github/actions/**.

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Fix codspeed' is vague and generic, lacking specificity about what aspect of codspeed is being fixed or what the actual change entails. Consider a more specific title that explains the fix, such as 'Ensure HDF5 availability for codspeed profiling' or 'Use micromamba environment for codspeed benchmarks to resolve HDF5 dependency'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description provides a clear problem statement (HDF5 version requirement) and the high-level solution approach (using micromamba), meeting the template requirements adequately despite no checklist items being marked.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_codspeed

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added the CI continuous integration label Mar 11, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b5e05fa97d

ℹ️ 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".

Comment on lines +41 to 42
prime_test_data_cache:
needs: setup

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate cache-priming job behind no_ci label

The new prime_test_data_cache job runs unconditionally on pull requests, but test_code is still guarded by if: ... !contains(..., 'no_ci'). On PRs labeled no_ci, this now runs a full 3-OS cache priming matrix anyway, and the workflow can still fail there even though tests were intentionally skipped. This regresses the existing “turn off CI” behavior and can block PRs or consume significant CI time unexpectedly.

Useful? React with 👍 / 👎.

Comment on lines +39 to 40
prime_test_data_cache:
needs: setup

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply no_ci guard to min-deps cache priming

Like the main test workflow, the added prime_test_data_cache job here is unconditional while test_code_min_deps is still skipped for no_ci PRs. That means PRs marked no_ci still execute the expensive cache-priming matrix (and can fail in it), which defeats the intended skip mechanism and introduces avoidable CI failures/cost.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/actions/cache-test-data/action.yml (1)

4-7: ⚠️ Potential issue | 🟠 Major

Don't let restore-keys bypass the manual cache reset.

Line 5 says cache-number is the reset knob, but Lines 62-63 still fall back to any data-${{ runner.os }}-${{ env.DATA_REGISTRY_HASH }}-* archive. Bumping the number will therefore copy the old cache forward instead of forcing a clean rebuild when you're trying to recover from stale or corrupted contents.

Suggested fix
       with:
         enableCrossOsArchive: true
         path: ${{ env.DATA_CACHE_PATH }}
         key: ${{ env.DATA_CACHE_KEY }}
-        restore-keys: |
-          data-${{ runner.os }}-${{ env.DATA_REGISTRY_HASH }}-

Also applies to: 62-63

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/actions/cache-test-data/action.yml around lines 4 - 7, The restore
behavior currently allows the wildcard fallback key data-${{ runner.os }}-${{
env.DATA_REGISTRY_HASH }}-* to be used even after bumping cache-number, so
bumping cache-number doesn't force a clean rebuild; update the cache action
configuration so the cache key includes the cache-number knob (cache-number) and
remove or adjust the wildcard restore-keys fallback so it also includes
cache-number (or omit the wildcard entirely) — ensure the primary key and any
restore-keys reference the cache-number symbol (and not only the
DATA_REGISTRY_HASH) so changing cache-number reliably invalidates prior caches.
🧹 Nitpick comments (1)
.github/workflows/prime_test_data_cache.yml (1)

3-63: Consider exposing this as a reusable workflow.

The same checkout/setup/install/restore/download/save sequence now exists in get_coverage.yml, runtests.yml, run_min_dep_tests.yml, test_doc_build.yml, and profile.yml, including a mix of hardcoded and env-driven cache-number values. Pulling this behind workflow_call would keep cache-key changes and future cache resets in one place.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/prime_test_data_cache.yml around lines 3 - 63, Extract the
repeated checkout/setup/install/restore/download/save sequence into a reusable
workflow by converting the current job "prime_test_data_cache" into a workflow
callable via workflow_call; create inputs for PYTHON_DEFAULT, cache-number (and
optional matrix.os or allow the caller to supply a matrix), and a boolean/enum
for whether to run the download/save steps, then replace the duplicated blocks
in get_coverage.yml, runtests.yml, run_min_dep_tests.yml, test_doc_build.yml,
and profile.yml to call the new reusable workflow (uses:
./.github/workflows/prime_test_data_cache.yml) and pass the required inputs;
ensure the job still references the same steps (actions/checkout@v4,
./.github/actions/load-shared-vars, actions/setup-python@v6, pip install -e .,
./.github/actions/cache-test-data, and .github/scripts/cache_test_data.py) so
cache-key logic and cache-number remain centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/profile.yml:
- Around line 65-70: The action metadata in
.github/actions/mamba-install-dascore/action.yml contains an unsupported field
for the input named install-package; open that action.yml, locate the inputs
section and remove the invalid "type: boolean" entry for install-package (and
any other "type" keys on inputs), leaving only supported keys like description,
required, default, or deprecationMessage so the workflow step using
./.github/actions/mamba-install-dascore validates correctly.

---

Outside diff comments:
In @.github/actions/cache-test-data/action.yml:
- Around line 4-7: The restore behavior currently allows the wildcard fallback
key data-${{ runner.os }}-${{ env.DATA_REGISTRY_HASH }}-* to be used even after
bumping cache-number, so bumping cache-number doesn't force a clean rebuild;
update the cache action configuration so the cache key includes the cache-number
knob (cache-number) and remove or adjust the wildcard restore-keys fallback so
it also includes cache-number (or omit the wildcard entirely) — ensure the
primary key and any restore-keys reference the cache-number symbol (and not only
the DATA_REGISTRY_HASH) so changing cache-number reliably invalidates prior
caches.

---

Nitpick comments:
In @.github/workflows/prime_test_data_cache.yml:
- Around line 3-63: Extract the repeated
checkout/setup/install/restore/download/save sequence into a reusable workflow
by converting the current job "prime_test_data_cache" into a workflow callable
via workflow_call; create inputs for PYTHON_DEFAULT, cache-number (and optional
matrix.os or allow the caller to supply a matrix), and a boolean/enum for
whether to run the download/save steps, then replace the duplicated blocks in
get_coverage.yml, runtests.yml, run_min_dep_tests.yml, test_doc_build.yml, and
profile.yml to call the new reusable workflow (uses:
./.github/workflows/prime_test_data_cache.yml) and pass the required inputs;
ensure the job still references the same steps (actions/checkout@v4,
./.github/actions/load-shared-vars, actions/setup-python@v6, pip install -e .,
./.github/actions/cache-test-data, and .github/scripts/cache_test_data.py) so
cache-key logic and cache-number remain centralized and consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2f406846-b9b0-4a53-8659-5a14c8051e61

📥 Commits

Reviewing files that changed from the base of the PR and between 5de00c6 and b5e05fa.

📒 Files selected for processing (8)
  • .github/actions/cache-test-data/action.yml
  • .github/scripts/cache_test_data.py
  • .github/workflows/get_coverage.yml
  • .github/workflows/prime_test_data_cache.yml
  • .github/workflows/profile.yml
  • .github/workflows/run_min_dep_tests.yml
  • .github/workflows/runtests.yml
  • .github/workflows/test_doc_build.yml

Comment thread .github/workflows/profile.yml
@codecov

codecov Bot commented Mar 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.48%. Comparing base (5de00c6) to head (d039b99).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #620   +/-   ##
=======================================
  Coverage   99.48%   99.48%           
=======================================
  Files         133      133           
  Lines       11494    11494           
=======================================
  Hits        11435    11435           
  Misses         59       59           
Flag Coverage Δ
unittests 99.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
.github/actions/cache-test-data/action.yml (1)

69-87: Consider the implications of enableCrossOsArchive with OS-specific paths.

The enableCrossOsArchive: true setting allows cache restoration across different OS types. However, since DATA_CACHE_PATH varies by OS (e.g., ~/.cache/dascore on Linux vs ~/Library/Caches/dascore on macOS), a cache created on one OS would be restored to a different path on another OS. This is likely fine if each OS job primes its own cache, but verify this is the intended behavior.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/actions/cache-test-data/action.yml around lines 69 - 87, The
workflow enables cross-OS caching via enableCrossOsArchive: true while
DATA_CACHE_PATH is OS-specific, which can cause caches created on one OS to be
restored to a different path; update the restore-test-data and cache-test-data
steps to either disable enableCrossOsArchive (set it to false) or make the cache
paths and keys OS-aware (e.g., include runner.os in DATA_CACHE_PATH or in
DATA_CACHE_KEY/restore-keys) so saved caches map to the correct platform; ensure
the change is applied for both uses of enableCrossOsArchive and references to
DATA_CACHE_PATH/DATA_CACHE_KEY.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/profile.yml:
- Around line 53-71: The benchmarks job depends on prime_test_data_cache but
never restores the cached test data; add a step named like "Restore test data
cache" at the start of the benchmarks job (before running setup/install steps
such as ./.github/actions/mamba-install-dascore) that uses actions/cache@v3 to
restore the same cache key and paths used by the prime_test_data_cache job
(match the key/paths/restore-keys used there so the cache is actually restored
into the test data directory or env var like TEST_DATA_CACHE_PATH).

---

Nitpick comments:
In @.github/actions/cache-test-data/action.yml:
- Around line 69-87: The workflow enables cross-OS caching via
enableCrossOsArchive: true while DATA_CACHE_PATH is OS-specific, which can cause
caches created on one OS to be restored to a different path; update the
restore-test-data and cache-test-data steps to either disable
enableCrossOsArchive (set it to false) or make the cache paths and keys OS-aware
(e.g., include runner.os in DATA_CACHE_PATH or in DATA_CACHE_KEY/restore-keys)
so saved caches map to the correct platform; ensure the change is applied for
both uses of enableCrossOsArchive and references to
DATA_CACHE_PATH/DATA_CACHE_KEY.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 830feadf-b827-4f99-9a3f-005c26cfe90d

📥 Commits

Reviewing files that changed from the base of the PR and between b5e05fa and e7cfa27.

📒 Files selected for processing (2)
  • .github/actions/cache-test-data/action.yml
  • .github/workflows/profile.yml

Comment on lines +53 to 71
benchmarks:
name: Run benchmarks
needs: prime_test_data_cache
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
fetch-tags: "true"
fetch-depth: '0'

- uses: ./.github/actions/load-shared-vars

- uses: ./.github/actions/mamba-install-dascore
with:
# Defined in .github/actions/load-shared-vars/action.yml
python-version: ${{ env.PYTHON_DEFAULT }}
install-group-str: "[profile]"
cache-number: 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing test data cache restore in benchmarks job.

The benchmarks job depends on prime_test_data_cache but doesn't actually restore the cached test data. GitHub Actions caches must be explicitly restored in each job that needs them—the needs directive only ensures job ordering, not cache sharing.

Add a step to restore the test data cache before running benchmarks:

🔧 Proposed fix to add cache restore
       - uses: ./.github/actions/load-shared-vars
 
+      - uses: ./.github/actions/cache-test-data
+        with:
+          mode: restore
+          cache-number: 1
+
       - uses: ./.github/actions/mamba-install-dascore
         with:
🧰 Tools
🪛 actionlint (1.7.11)

[error] 65-65: 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)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/profile.yml around lines 53 - 71, The benchmarks job
depends on prime_test_data_cache but never restores the cached test data; add a
step named like "Restore test data cache" at the start of the benchmarks job
(before running setup/install steps such as
./.github/actions/mamba-install-dascore) that uses actions/cache@v3 to restore
the same cache key and paths used by the prime_test_data_cache job (match the
key/paths/restore-keys used there so the cache is actually restored into the
test data directory or env var like TEST_DATA_CACHE_PATH).

@codspeed-hq

codspeed-hq Bot commented Mar 11, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 24.59%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 27 improved benchmarks
✅ 27 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
test_wiggle 303.3 ms 258.4 ms +17.39%
test_transpose 4.2 ms 3.4 ms +23.06%
test_contiguous_merge 220.8 ms 195 ms +13.22%
test_scan_df 478.7 ms 407.1 ms +17.58%
test_taper 12.3 ms 11.1 ms +10.86%
test_last 6.3 ms 5.2 ms +21.5%
test_max 14.9 ms 12.6 ms +17.52%
test_differentiate 33.8 ms 27.1 ms +24.49%
test_no_overlap_merge 265.7 ms 217.4 ms +22.2%
test_scan 347.8 ms 299.7 ms +16.06%
test_select_half_range 49.7 ms 40.3 ms +23.07%
test_diverse_merge 697.1 ms 607.6 ms +14.73%
test_select_full_range 69.9 ms 56.6 ms +23.6%
test_min 14.9 ms 12.6 ms +17.53%
test_1second_chunk 1,219 ms 979.1 ms +24.51%
test_definite_integrate 34 ms 27.9 ms +21.78%
test_select_string_match 25.5 ms 21 ms +21.29%
test_select 26.6 ms 21.4 ms +24.13%
test_half_second_chunk 2.4 s 1.9 s +24.59%
test_align_1d_shift_valid 22.9 ms 20.2 ms +13.5%
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.


Comparing fix_codspeed (8c4ac64) with master (5de00c6)

Open in CodSpeed

@d-chambers d-chambers added the benchmark Run the benchmark suite label Mar 11, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
.github/workflows/runtests.yml (1)

85-116: ⚠️ Potential issue | 🟠 Major

Missing test data cache restore in test_code job.

The test_code job depends on prime_test_data_cache but doesn't restore the cached test data. GitHub Actions caches must be explicitly restored in each job that needs them—the needs directive only ensures job ordering, not cache sharing.

Add a step to restore the test data cache after checkout:

🔧 Proposed fix to add cache restore
       - uses: actions/checkout@v4
         with:
           fetch-tags: 'true'
           fetch-depth: '0'
 
+      - uses: ./.github/actions/cache-test-data
+        with:
+          mode: restore
+          cache-number: ${{ env.CACHE_NUMBER }}
+
       - uses: ./.github/actions/mamba-install-dascore
         with:
           python-version: ${{ matrix.python-version }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/runtests.yml around lines 85 - 116, The test_code job
depends on prime_test_data_cache but never restores the cached test data; add a
cache-restore step immediately after the actions/checkout step in the test_code
job to restore the same cache key used by prime_test_data_cache so test data is
available when running ./.github/test_code.sh; reference the job name test_code,
the dependency prime_test_data_cache, and the existing checkout step to locate
where to insert the restore step.
.github/workflows/run_min_dep_tests.yml (1)

81-109: ⚠️ Potential issue | 🟠 Major

Missing test data cache restore in test_code_min_deps job.

Same issue as runtests.yml—the job depends on prime_test_data_cache but doesn't restore the cached test data. Add a cache restore step after checkout.

🔧 Proposed fix to add cache restore
       - uses: actions/checkout@v4
         with:
           fetch-tags: "true"
           fetch-depth: '0'
 
+      - uses: ./.github/actions/cache-test-data
+        with:
+          mode: restore
+          cache-number: 1
+
       - uses: ./.github/actions/mamba-install-dascore
         with:
           python-version: ${{ matrix.python-version }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/run_min_dep_tests.yml around lines 81 - 109, The
test_code_min_deps job is missing a restore of the primed test data cache; add a
cache-restore step immediately after the actions/checkout@v4 step in the
test_code_min_deps job to restore the artifacts created by the
prime_test_data_cache job (use the same cache key/restore logic used in
runtests.yml or the prime_test_data_cache output), so the job actually gets the
prepared test data before running the mamba install and "Install dascore (min
deps)" steps.
♻️ Duplicate comments (2)
.github/workflows/profile.yml (2)

62-91: ⚠️ Potential issue | 🟠 Major

Missing test data cache restore in benchmarks job.

The benchmarks job depends on prime_test_data_cache but doesn't restore the cached test data. Add a cache restore step after checkout and before running benchmarks.

🔧 Proposed fix to add cache restore
       - uses: ./.github/actions/load-shared-vars
 
+      - uses: ./.github/actions/cache-test-data
+        with:
+          mode: restore
+          cache-number: 1
+
       - uses: ./.github/actions/mamba-install-dascore
         with:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/profile.yml around lines 62 - 91, The benchmarks job
declares a dependency on prime_test_data_cache but never restores that cache;
add a cache-restore step immediately after the actions/checkout step (and before
running the benchmarks) that uses the same cache key/paths produced by the
prime_test_data_cache job (use actions/cache@v3 or your existing cache action),
so the test data populated by prime_test_data_cache is available to the Run
benchmarks step (refer to the benchmarks job, the prime_test_data_cache
dependency, and the Run benchmarks step/micromamba run command to place the
restore).

77-82: ⚠️ Potential issue | 🟠 Major

Invalid metadata in mamba-install-dascore action blocks workflow parsing.

The static analysis tool reports that .github/actions/mamba-install-dascore/action.yml contains an unsupported type: boolean field for the install-package input. GitHub Actions action metadata only supports description, required, default, and deprecationMessage for inputs. This needs to be fixed in the action definition file (not included in this PR).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/profile.yml around lines 77 - 82, The action metadata for
the mamba-install-dascore action has an unsupported "type: boolean" on the
install-package input; remove the "type" key from the install-package input in
the action metadata and instead represent a boolean default as a string default
(e.g., default: "true"/"false") or omit it and use required/default only, making
sure only the allowed input keys (description, required, default,
deprecationMessage) are present; update the install-package input entry in the
mamba-install-dascore action metadata accordingly so the workflow parser accepts
it.
🧹 Nitpick comments (1)
.github/workflows/run_min_dep_tests.yml (1)

16-19: Consider adding CACHE_NUMBER env for consistency.

This workflow uses hardcoded cache-number: 1 (lines 67, 79) while runtests.yml defines a CACHE_NUMBER env variable. Consider adding the same env variable for easier cache invalidation across workflows.

♻️ Proposed fix for consistency
 env:
+  # used to manually trigger cache reset. Just increment if needed.
+  CACHE_NUMBER: 1
   # Ensure matplotlib doesn't try to show figures in CI
   MPLBACKEND: Agg

Then update lines 67 and 79 to use ${{ env.CACHE_NUMBER }}.

Also applies to: 67-67, 79-79

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/run_min_dep_tests.yml around lines 16 - 19, Add a
CACHE_NUMBER environment variable to the workflow env block (next to MPLBACKEND
and QT_QPA_PLATFORM) and set it to the same default used in runtests.yml (e.g.,
1); then replace the hardcoded cache-number: 1 occurrences with cache-number:
${{ env.CACHE_NUMBER }} so the workflow uses the shared CACHE_NUMBER value
(search for CACHE_NUMBER, MPLBACKEND, QT_QPA_PLATFORM, and the two cache-number
entries to locate the spots to update).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In @.github/workflows/run_min_dep_tests.yml:
- Around line 81-109: The test_code_min_deps job is missing a restore of the
primed test data cache; add a cache-restore step immediately after the
actions/checkout@v4 step in the test_code_min_deps job to restore the artifacts
created by the prime_test_data_cache job (use the same cache key/restore logic
used in runtests.yml or the prime_test_data_cache output), so the job actually
gets the prepared test data before running the mamba install and "Install
dascore (min deps)" steps.

In @.github/workflows/runtests.yml:
- Around line 85-116: The test_code job depends on prime_test_data_cache but
never restores the cached test data; add a cache-restore step immediately after
the actions/checkout step in the test_code job to restore the same cache key
used by prime_test_data_cache so test data is available when running
./.github/test_code.sh; reference the job name test_code, the dependency
prime_test_data_cache, and the existing checkout step to locate where to insert
the restore step.

---

Duplicate comments:
In @.github/workflows/profile.yml:
- Around line 62-91: The benchmarks job declares a dependency on
prime_test_data_cache but never restores that cache; add a cache-restore step
immediately after the actions/checkout step (and before running the benchmarks)
that uses the same cache key/paths produced by the prime_test_data_cache job
(use actions/cache@v3 or your existing cache action), so the test data populated
by prime_test_data_cache is available to the Run benchmarks step (refer to the
benchmarks job, the prime_test_data_cache dependency, and the Run benchmarks
step/micromamba run command to place the restore).
- Around line 77-82: The action metadata for the mamba-install-dascore action
has an unsupported "type: boolean" on the install-package input; remove the
"type" key from the install-package input in the action metadata and instead
represent a boolean default as a string default (e.g., default: "true"/"false")
or omit it and use required/default only, making sure only the allowed input
keys (description, required, default, deprecationMessage) are present; update
the install-package input entry in the mamba-install-dascore action metadata
accordingly so the workflow parser accepts it.

---

Nitpick comments:
In @.github/workflows/run_min_dep_tests.yml:
- Around line 16-19: Add a CACHE_NUMBER environment variable to the workflow env
block (next to MPLBACKEND and QT_QPA_PLATFORM) and set it to the same default
used in runtests.yml (e.g., 1); then replace the hardcoded cache-number: 1
occurrences with cache-number: ${{ env.CACHE_NUMBER }} so the workflow uses the
shared CACHE_NUMBER value (search for CACHE_NUMBER, MPLBACKEND, QT_QPA_PLATFORM,
and the two cache-number entries to locate the spots to update).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6f665423-7a12-4bca-b594-099c9e3ec0d1

📥 Commits

Reviewing files that changed from the base of the PR and between e7cfa27 and 8c4ac64.

📒 Files selected for processing (3)
  • .github/workflows/profile.yml
  • .github/workflows/run_min_dep_tests.yml
  • .github/workflows/runtests.yml

@d-chambers d-chambers removed the benchmark Run the benchmark suite label Mar 11, 2026
@d-chambers
d-chambers merged commit 1cc21db into master Mar 11, 2026
58 checks passed
@d-chambers
d-chambers deleted the fix_codspeed branch March 11, 2026 16:23
This was referenced Mar 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI continuous integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant