feat(perf): persist release report artifacts for rerendering - #206
Conversation
- Store deterministic comparison CSV with an adjacent schema-versioned provenance sidecar before temporary worktrees are removed. - Render and promote reports only from validated artifact reloads, with fail-closed path checks and transactional rollback. - Add performance-rerender and document artifact retention, GitHub assets, and the release workflow. - Refresh Rust, Python, contributor-tool, and GitHub Action pins, and separate Dependabot security update groups. Closes #205
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe release performance pipeline now persists schema-versioned CSV and provenance artifacts, validates and publishes them transactionally, renders reports from reloaded artifacts, and supports rerendering without benchmarks or temporary worktrees. CI pins, tooling versions, documentation, and tests were updated. ChangesRelease performance reporting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant BenchCompare
participant PerformanceArtifacts
participant ArchivePerformance
participant ReleaseReport
BenchCompare->>PerformanceArtifacts: Build and validate CSV/provenance bundle
PerformanceArtifacts->>PerformanceArtifacts: Serialize and publish artifact pair
ArchivePerformance->>PerformanceArtifacts: Reload published artifacts
PerformanceArtifacts-->>ArchivePerformance: Return validated bundle
ArchivePerformance->>ReleaseReport: Render Markdown from retained bundle
ReleaseReport-->>ArchivePerformance: Return validated report
ArchivePerformance->>ArchivePerformance: Promote report and archive with rollback protection
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #206 +/- ##
==========================================
+ Coverage 97.84% 97.86% +0.02%
==========================================
Files 8 8
Lines 4969 4969
==========================================
+ Hits 4862 4863 +1
+ Misses 107 106 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
scripts/bench_compare.py (2)
1368-1394: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare the suite and scope selection rules with the comparison collector.
_unavailable_artifact_rowsre-implements three selection rules that_collect_comparisonsalready owns: the suite membership test, therelease-signalexact-group filter, and therelease-signalvs_linalg bench filter. Line 1382 also re-derives the suite from theexact_group-name prefix instead of the shared suite mapping.If any selection rule changes in one place, the retained coverage rows and the comparison rows disagree, and the artifact then misreports which baselines were excluded. Extract the row-selection predicate into one helper and call it from both paths.
🤖 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/bench_compare.py` around lines 1368 - 1394, The selection logic in _unavailable_artifact_rows must be shared with _collect_comparisons instead of duplicated. Extract a common row-selection predicate that uses the existing suite mapping and encapsulates suite membership, release-signal exact-group filtering, and release-signal vs_linalg bench filtering; then call it from both paths, removing the local row_suite derivation and checks.
1953-1976: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the artifact path resolution and publication from
main.
mainalready suppressesC901,PLR0911,PLR0912, andPLR0915. This change adds two more early-return groups and repeats the same paired-option test three times, at line 1953, line 1961, and line 2066. Line 1953 already proves the two options agree, so the later tests only serve to narrow the types.Extract two helpers, for example
_resolve_artifact_paths(args, root, output_path) -> ArtifactPaths | Noneand_write_and_render_artifacts(...) -> str. Thenmaintestsartifact_paths is not Noneonce, and the invariantAssertionErrorat line 2068 becomes unnecessary.Also applies to: 2066-2085
🤖 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/bench_compare.py` around lines 1953 - 1976, Extract artifact path resolution from main into a helper such as _resolve_artifact_paths, preserving paired-option validation, path normalization, distinct-path checks, and existing error returns. Extract artifact writing and rendering around the later block into a helper such as _write_and_render_artifacts, then have main branch once on artifact_paths is not None and remove the redundant paired-option narrowing and invariant AssertionError.scripts/tests/test_performance_artifacts.py (1)
146-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the CSV digest and row-count mismatch guards.
_replace_csv_and_digestrecomputescsv.sha256for every malformed-CSV fixture. As a result, no test reaches the two fail-closed branches inload_bundle_bytes: the digest mismatch and the row-count mismatch. Both branches detect tampered or partially written pairs, which the artifact contract treats as a hard failure.Add two focused tests that mutate the CSV without repairing the sidecar, and that change
csv.row_count.💚 Proposed tests for the mismatch guards
def test_artifact_loader_rejects_csv_digest_mismatch() -> None: csv_payload, provenance_payload = serialize_bundle(_bundle()) tampered = csv_payload.replace(b"comparable", b"comparable", 1) + b"\n" with pytest.raises(ValueError, match="CSV digest mismatch"): load_bundle_bytes(tampered, provenance_payload, source="digest mismatch fixture") def test_artifact_loader_rejects_csv_row_count_mismatch() -> None: csv_payload, provenance_payload = serialize_bundle(_bundle()) provenance = json.loads(provenance_payload) provenance["csv"]["row_count"] = 99 with pytest.raises(ValueError, match="CSV row count mismatch"): load_bundle_bytes( csv_payload, (json.dumps(provenance, indent=2, sort_keys=True) + "\n").encode(), source="row count mismatch fixture", )Also applies to: 205-211
🤖 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_performance_artifacts.py` around lines 146 - 149, The performance artifact tests lack coverage for the fail-closed CSV integrity guards. Add focused tests near the existing artifact-loader tests that mutate the CSV without updating its sidecar and assert load_bundle_bytes raises ValueError for a CSV digest mismatch, then alter provenance["csv"]["row_count"] and assert the corresponding row-count mismatch error, while leaving _replace_csv_and_digest unchanged for fixtures that intentionally repair the digest.scripts/archive_performance.py (1)
390-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the "How to Update" block with
bench_compare.This PR edits the same instruction block twice: here and in
scripts/bench_compare.py_generate_markdown(thejust performance-rerenderlines and the retained-artifacts sentence). The two copies must stay identical, but nothing enforces that._normalize_how_to_updatesilently rewrites the section during promotion, so a drift produces a local report that differs from the promoted report without any test failure.
archive_performancealready imports frombench_compare. Move the block into one shared constant or helper and use it in both modules.🤖 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/archive_performance.py` around lines 390 - 402, Extract the duplicated “How to Update” instruction block into a shared constant or helper in the existing bench_compare import path, including the rerender command and retained-artifacts sentence. Update archive_performance and bench_compare._generate_markdown to consume that shared source, and ensure _normalize_how_to_update preserves the same content during promotion.scripts/tests/test_bench_compare.py (1)
1038-1043: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the two new artifact CLI guards.
The parameterized test covers invalid path combinations. Two other new fail-closed guards in
mainhave no test: the paired-option check for--csv-outputand--provenance-output, and the median-baseline restriction that rejects--snapshotor a non-median--stat. Both return exit code 2 with distinct messages.Extend the parameterization or add two short tests so a regression in either guard fails the suite.
💚 Proposed tests for the CLI guards
`@pytest.mark.parametrize`( ("extra_args", "expected"), [ (["--csv-output", "performance.csv"], "must be provided together"), ( ["--csv-output", "performance.csv", "--provenance-output", "performance.provenance.json", "--snapshot"], "require a median baseline comparison", ), ], ) def test_main_rejects_incomplete_artifact_selection( tmp_path: Path, capsys: pytest.CaptureFixture[str], extra_args: list[str], expected: str, ) -> None: rc = bench_compare.main(["v0.4.3", "--repo-root", str(tmp_path), *extra_args]) assert rc == 2 assert expected in capsys.readouterr().err🤖 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_bench_compare.py` around lines 1038 - 1043, Extend the CLI guard tests around test_main_rejects_invalid_artifact_paths_without_writing, or add a focused parameterized test, to cover incomplete --csv-output/--provenance-output pairing and artifact selection with --snapshot or a non-median --stat. Invoke bench_compare.main with each invalid argument set and assert exit code 2 plus the corresponding distinct error message.
🤖 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/RELEASING.md`:
- Around line 155-157: Update the performance comparison wording in the release
documentation to say “stored GitHub Release assets” instead of “stored GitHub
Actions release assets,” preserving the existing command guidance and
terminology distinction elsewhere in the file.
---
Nitpick comments:
In `@scripts/archive_performance.py`:
- Around line 390-402: Extract the duplicated “How to Update” instruction block
into a shared constant or helper in the existing bench_compare import path,
including the rerender command and retained-artifacts sentence. Update
archive_performance and bench_compare._generate_markdown to consume that shared
source, and ensure _normalize_how_to_update preserves the same content during
promotion.
In `@scripts/bench_compare.py`:
- Around line 1368-1394: The selection logic in _unavailable_artifact_rows must
be shared with _collect_comparisons instead of duplicated. Extract a common
row-selection predicate that uses the existing suite mapping and encapsulates
suite membership, release-signal exact-group filtering, and release-signal
vs_linalg bench filtering; then call it from both paths, removing the local
row_suite derivation and checks.
- Around line 1953-1976: Extract artifact path resolution from main into a
helper such as _resolve_artifact_paths, preserving paired-option validation,
path normalization, distinct-path checks, and existing error returns. Extract
artifact writing and rendering around the later block into a helper such as
_write_and_render_artifacts, then have main branch once on artifact_paths is not
None and remove the redundant paired-option narrowing and invariant
AssertionError.
In `@scripts/tests/test_bench_compare.py`:
- Around line 1038-1043: Extend the CLI guard tests around
test_main_rejects_invalid_artifact_paths_without_writing, or add a focused
parameterized test, to cover incomplete --csv-output/--provenance-output pairing
and artifact selection with --snapshot or a non-median --stat. Invoke
bench_compare.main with each invalid argument set and assert exit code 2 plus
the corresponding distinct error message.
In `@scripts/tests/test_performance_artifacts.py`:
- Around line 146-149: The performance artifact tests lack coverage for the
fail-closed CSV integrity guards. Add focused tests near the existing
artifact-loader tests that mutate the CSV without updating its sidecar and
assert load_bundle_bytes raises ValueError for a CSV digest mismatch, then alter
provenance["csv"]["row_count"] and assert the corresponding row-count mismatch
error, while leaving _replace_csv_and_digest unchanged for fixtures that
intentionally repair the digest.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9fb96a9-cd88-43dc-b5aa-a5eca1bca002
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/actions/setup-just/action.yml.github/dependabot.yml.github/workflows/audit.yml.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/codecov.yml.github/workflows/codeql.yml.github/workflows/release-benchmarks.yml.github/workflows/rust-clippy.yml.github/workflows/semgrep-sarif.ymlCONTRIBUTING.mdCargo.tomlREADME.mddocs/BENCHMARKING.mddocs/RELEASING.mdjustfilepyproject.tomlscripts/README.mdscripts/archive_performance.pyscripts/bench_compare.pyscripts/performance_artifacts.pyscripts/tests/test_archive_performance.pyscripts/tests/test_bench_compare.pyscripts/tests/test_performance_artifacts.py
- Treat CRLF and LF archive reports as equivalent while preserving stored bytes. - Share comparison selection and report-update guidance across generation paths. - Isolate artifact path resolution and retained-artifact rendering. - Clarify that durable benchmark downloads are GitHub Release assets.
1 participant
Closes #205
Summary by CodeRabbit
New Features
performance-rerenderto regenerate release reports without rerunning benchmarks.Documentation
Chores