chore: add pytest-benchmem benchmark suite with PR head-vs-base reporting#108
chore: add pytest-benchmem benchmark suite with PR head-vs-base reporting#108FBumann wants to merge 4 commits into
Conversation
…ting Benchmark aggregate() under the production config focus (hierarchical, Distribution scope=global, include_period_sums=False, extremes replace): a deterministic representation x extremes grid, the representation x columns interaction, preserve_column_means, and column/slice/day scaling. The Benchmarks workflow adapts fluxopt's sticky-comment reporting: it runs the suite on the PR head and base in the same job, posts a benchmem delta table (walltime + memray peak) as one sticky PR comment, and uploads an interactive benchmem plot artifact. Informational only, never blocks a merge. Local baselines land in .benchmarks/ (gitignored, machine-specific). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a parameterized ChangesAggregate benchmarks
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant pytestBenchmem
participant BaseRevision
participant BenchmarkReport
GitHubActions->>pytestBenchmem: Run aggregate benchmarks for PR head
GitHubActions->>BaseRevision: Checkout base and overlay benchmarks
BaseRevision->>pytestBenchmem: Run aggregate benchmarks for base
pytestBenchmem->>BenchmarkReport: Provide benchmark JSON results
BenchmarkReport->>GitHubActions: Generate delta table and plot
GitHubActions->>GitHubActions: Upload plot and update sticky comment
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 5
🤖 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 @.github/workflows/benchmarks.yaml:
- Around line 77-79: Update the benchmark summary-generation branch in the
workflow to use standalone-run wording when triggered by workflow_dispatch,
rather than claiming a “Δ% vs base” comparison. Preserve the existing
base-comparison wording for runs where base.json is available, and keep the
surrounding runner-noise and artifact notes appropriate to both branches.
- Around line 25-30: Split the benchmark workflow so comparison, summary, and
plot reporting run for every pull request, including forks, without the current
repository-origin guard. Keep only sticky-comment publishing behind the
same-repository check, and place that step in a separate job with pull-requests:
write permissions; preserve continue-on-error for informational benchmark
results.
- Around line 35-37: Update the actions/checkout step in the benchmarks workflow
to set persist-credentials to false, preventing the PR-write token from being
stored in .git/config while preserving the existing fetch-depth setting and
explicit GH_TOKEN usage in later steps.
In `@benchmarks/test_bench_aggregate.py`:
- Around line 156-159: Add a 365-day case to the n_days parameterization of
test_full_scale_days, retaining the existing 730-day case so the benchmark
produces a day-scaling comparison.
- Line 27: Update make_data() to instantiate a new NumPy random generator seeded
with 42 for each dataset creation, and use that local generator instead of the
module-level RNG. Remove or stop using the global RNG so each benchmark case
produces identical data regardless of test selection or execution order.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6aab7866-59a6-44c9-a28b-89baacdec665
📒 Files selected for processing (4)
.github/workflows/benchmarks.yaml.gitignorebenchmarks/README.mdbenchmarks/test_bench_aggregate.py
| # Commenting needs a writable token; fork PRs only get a read-only one. | ||
| if: >- | ||
| ${{ github.event_name != 'pull_request' || | ||
| github.event.pull_request.head.repo.full_name == github.repository }} | ||
| runs-on: ubuntu-24.04 | ||
| continue-on-error: true # informational, never blocks a merge |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Run benchmark reports for fork PRs too.
The job guard skips the comparison, summary, and plot for every fork PR, not just the sticky comment. Split this into a read-only report job for all PRs and a same-repository comment job with pull-requests: write.
Also applies to: 94-108
🤖 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 @.github/workflows/benchmarks.yaml around lines 25 - 30, Split the benchmark
workflow so comparison, summary, and plot reporting run for every pull request,
including forks, without the current repository-origin guard. Keep only
sticky-comment publishing behind the same-repository check, and place that step
in a separate job with pull-requests: write permissions; preserve
continue-on-error for informational benchmark results.
BenchmarksΔ% vs base, one table sorted by biggest change first Full tablebenchmarks/test_bench_aggregate.py
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ruff-action tracks the latest ruff, which now formats Python code blocks inside markdown and rejects aligned trailing comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Seed the RNG per make_data call so each case's data is independent of test selection and execution order. Add a 365-day case so the day axis scales within its own test. Report one severity-sorted table (group by module, sort by change). Run fork PRs too — only the sticky comment needs a writable token, so only that step is repo-guarded — and stop persisting the checkout token since benchmarks execute PR code. Use standalone wording when there is no base run to compare against. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Superseded by #109, which includes these commits plus the suite restructure. |
1 participant
Summary
benchmarks/— pytest-benchmem suite foraggregate()focused on the production config (hierarchical,Distribution(scope="global"),include_period_sums=False, extremesreplace): a deterministic representation × extremes grid, the representation × columns interaction,preserve_column_meanson/off, and column/slice/day scaling under the full config..github/workflows/benchmarks.yaml— adapted from fluxopt's benchmark-hint workflow: runs the suite on PR head and base in the same job, keeps one sticky PR comment with thebenchmem compare --difftable (walltime + memray peak), writes it to the step summary, and uploads an interactivebenchmem plotartifact.continue-on-error, so it never blocks a merge..benchmarks/gitignored — local baselines are machine-specific.Baseline findings that motivated the case selection
Distribution(scope="global")is the only expensive focus option: 2–2.6× over medoid at 8 columns, 3× at 128 columns (durationRepresentation's global branch is an unvectorized per-attribute loop — upstream tsam issue).extremes replace,dist_cluster,mean, andpreserve_column_meansare all free within noise.n_clustersis flat.Cases are a deterministic grid rather than fuzzed because
benchmem comparematches runs by test ID, so IDs must be stable for baselines and the PR diff to pair up.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores