Perf/436 release benchmark workflow#514
Conversation
- Add release-signal benchmark recipes for saved Criterion baselines, local release comparisons, GitHub Release asset comparisons, and curated report promotion. - Package release benchmark artifacts with raw Criterion data, metadata, baseline results, and human-readable performance summaries. - Document the invariant-first benchmarking model so performance evidence is only publishable when scientific invariants are maintained. Refs #436
- Reject non-finite, zero, and negative Criterion point estimates before rendering saved-baseline reports. - Clarify saved-baseline docs so `bench-save-last` pairs with `bench-compare last`, while explicit tag baselines remain opt-in.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR reframes benchmarking around scientific invariants, extends benchmark CI execution, adds new benchmark and release-performance commands, implements report generation and archiving, and updates tests and documentation to match the new workflow. ChangesRelease-performance benchmarking workflow
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Maintainer
participant justfile
participant benchmark_utils
participant Git
participant Criterion
participant docs_archive
Maintainer->>justfile: just performance-release current_tag baseline_tag
justfile->>benchmark_utils: dispatch performance-release
benchmark_utils->>Git: resolve tags and create worktree
benchmark_utils->>Criterion: run benchmarks and compare estimates
benchmark_utils->>docs_archive: promote report and update archive index
docs_archive-->>Maintainer: archived release comparison
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage ∅ diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ ∅ diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (282a72d) 80105 72958 91.08% Head commit (178e9a1) 80105 (+0) 72958 (+0) 91.08% (+0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#514) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
justfile (1)
907-936: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate tag-pair validation logic between
performance-github-assetsandperformance-release.Both recipes repeat the identical "both-or-neither" check for
current_tag/baseline_tag. Consider extracting the validation into a private helper recipe to avoid drift if the validation logic changes later.♻️ Proposed refactor sketch
+_require-tag-pair current_tag baseline_tag: + #!/usr/bin/env bash + set -euo pipefail + current_tag="{{ current_tag }}" + baseline_tag="{{ baseline_tag }}" + if [[ -n "$current_tag" || -n "$baseline_tag" ]]; then + if [[ -z "$current_tag" || -z "$baseline_tag" ]]; then + echo "current_tag and baseline_tag must be provided together" >&2 + exit 2 + fi + fi performance-github-assets current_tag="" baseline_tag="": _ensure-uv #!/usr/bin/env bash set -euo pipefail - current_tag="{{ current_tag }}" - baseline_tag="{{ baseline_tag }}" - if [[ -n "$current_tag" || -n "$baseline_tag" ]]; then - if [[ -z "$current_tag" || -z "$baseline_tag" ]]; then - echo "current_tag and baseline_tag must be provided together" >&2 - exit 2 - fi - uv run benchmark-utils performance-github-assets "$current_tag" "$baseline_tag" - else - uv run benchmark-utils performance-github-assets - fi + just _require-tag-pair "{{ current_tag }}" "{{ baseline_tag }}" + if [[ -n "{{ current_tag }}" ]]; then + uv run benchmark-utils performance-github-assets "{{ current_tag }}" "{{ baseline_tag }}" + else + uv run benchmark-utils performance-github-assets + fi🤖 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 `@justfile` around lines 907 - 936, Both justfile recipes duplicate the same current_tag/baseline_tag “both-or-neither” validation, so extract that check into a shared private helper recipe and call it from performance-github-assets and performance-release. Keep the existing behavior and error message the same, but centralize the tag-pair validation so future changes only need to be made once.scripts/tests/test_benchmark_utils.py (1)
268-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
collect_criterion_comparisonscall in this test.No estimates are written before Line 268, so
collect_criterion_comparisonsalways returns an empty list and theif not comparisonsfallback is always used. The call adds no coverage and makes the fixture setup misleading — either drop it and build the comparison list directly, or write matchingnew/lastestimates so the collection path is actually exercised.🤖 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 `@scripts/tests/test_benchmark_utils.py` around lines 268 - 276, The test is relying on a dead collect_criterion_comparisons call because no Criterion estimates are created beforehand, so the fallback is always taken. In the affected test, either remove the collect_criterion_comparisons invocation and construct the CriterionComparison list directly, or update the fixture setup to write matching new/last estimates before calling collect_criterion_comparisons so the collection path is truly exercised. Use the existing test body around benchmark_utils.CriterionComparison and collect_criterion_comparisons to locate the change.
🤖 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.
Nitpick comments:
In `@justfile`:
- Around line 907-936: Both justfile recipes duplicate the same
current_tag/baseline_tag “both-or-neither” validation, so extract that check
into a shared private helper recipe and call it from performance-github-assets
and performance-release. Keep the existing behavior and error message the same,
but centralize the tag-pair validation so future changes only need to be made
once.
In `@scripts/tests/test_benchmark_utils.py`:
- Around line 268-276: The test is relying on a dead
collect_criterion_comparisons call because no Criterion estimates are created
beforehand, so the fallback is always taken. In the affected test, either remove
the collect_criterion_comparisons invocation and construct the
CriterionComparison list directly, or update the fixture setup to write matching
new/last estimates before calling collect_criterion_comparisons so the
collection path is truly exercised. Use the existing test body around
benchmark_utils.CriterionComparison and collect_criterion_comparisons to locate
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bf2a4714-55d4-487e-aa7c-99b194b74b49
📒 Files selected for processing (14)
.github/workflows/release-benchmarks.ymlAGENTS.mdREADME.mdbenches/README.mdbenches/ci_performance_suite.rsdocs/RELEASING.mddocs/archive/performance/README.mddocs/dev/commands.mddocs/dev/perf-tuning.mddocs/dev/tooling-alignment.mdjustfilescripts/README.mdscripts/benchmark_utils.pyscripts/tests/test_benchmark_utils.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
=======================================
Coverage 91.05% 91.05%
=======================================
Files 88 88
Lines 79884 79884
=======================================
Hits 72739 72739
Misses 7145 7145
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
- Centralize explicit tag-pair validation for release performance recipes. - Clarify that `perf-no-regressions` reuses cached same-machine main baselines. - Exercise GitHub Release asset comparisons through real Criterion samples. Closes #436
1 participant
No description provided.