Repository navigation
fix(ci): make wall-clock time advisory in the performance gate - #338
Merged
Merged
Conversation
#337 switched the gate to best-of-N and warned only on the network-bound benchmarks, but the first post-merge run (39243572918) still failed: `apps` (system_profiler) measured +104% best-of-3 against the 2026-10-11 baseline, after +8% on 2026-10-18, with no runtime change. Wall-clock on shared macOS runners cannot tell a regression from runner noise. - Relative time changes are warnings ("time is advisory"). - The gate fails on what noise cannot produce: a memory regression beyond 20%, or a benchmark whose best iteration (min_time, else avg_time) exceeds the 30 s ceiling — also enforced when there is no baseline. - tests/test_compare_performance.py: 8 tests incl. replays of the 10-18 and 10-19 runs; 6 of them fail on the #337 version. - tests/test_project_consistency.py: locate the regression-check step by its `python scripts/compare_performance.py` invocation, not its display name (the step label now states the new rule). Full suite: 2841 passed, 16 skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
Reviewer's GuideThe PR changes the performance gate from wall-clock regression enforcement to advisory time reporting, while preserving hard failures for memory regressions and best-of-N runtimes over 30 seconds; tests, workflow metadata, consistency checks, and changelog documentation are updated accordingly. Flow diagram for the advisory performance gateflowchart TD
Results[Performance results] --> Limits[Check best time and memory limits]
Limits -->|Best time over 30s| Fail[Fail gate]
Limits -->|Memory over 500MB| WarnMemory[Warn only]
Limits --> Baseline{Baseline available?}
Baseline -->|No| Pass[Pass gate]
Baseline -->|Yes| Compare[Compare against baseline]
Compare -->|Memory regression over 20%| Fail
Compare -->|Time regression| WarnTime[Warn: time is advisory]
Compare -->|No gated regression| Pass
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Mon Oct 19 17:59:17 UTC 2026 Bandit Security ScanPip-Audit Results |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70a70e92a1
ℹ️ 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
+78
to
+79
| if label == "time": | ||
| warnings.append(f" WARNING: {detail}; time is advisory (shared-runner noise)") |
There was a problem hiding this comment.
When a real runtime regression remains below 30 seconds, this now makes it advisory even though the purported memory fallback cannot currently detect anything: scripts/performance_test.py:51-64 calls disable_profiling() before generate_report(), while PerformanceProfiler.report() in versiontracker/profiling.py:201-208 returns an empty dict whenever profiling is disabled, so every produced avg_memory_mb is zero. Consequently ordinary regressions always pass and can be promoted as the next baseline; either fix the producer's memory measurement in this change or retain a gated relative signal.
Useful? React with 👍 / 👎.
| retention-days: 30 | ||
|
|
||
| - name: Compare with baseline (fail on >20% regression) | ||
| - name: Compare with baseline (fails on memory regression or a best time over 30s; time deltas advisory) |
There was a problem hiding this comment.
After this behavior change, CONTRIBUTING.md:281-292 still tells contributors that relative avg_time regressions fail the workflow and that operations over 30 seconds are warning-only. The new gate does the opposite for both cases, so anyone using the repository's performance instructions will misinterpret results and acceptance criteria; update that section alongside the workflow label.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a follow-up to #337. That PR moved the weekly performance gate to best-of-N and made
recommend/outdatedwarn-only. The first run after it merged (dispatched run 39243572918) still failed, onapps:appsbest-of-3recommendbest-of-3outdatedbest-of-3There was no runtime change.
appsshells out tosystem_profiler, whose duration on a fresh macOS CI VM depends on the machine's state. #337 was validated on only one pair of runs, and that pair happened to be quiet. Wall-clock time on shared runners can't tell a regression from runner noise.Changes
scripts/compare_performance.py:min_time, falling back toavg_time) exceeds the 30 s ceiling. The ceiling is enforced even when there's no baseline. One slow iteration can't trip it.tests/test_compare_performance.py: 8 tests, including replays of the 10-18 and 10-19 runs, which now pass with warnings. 6 of them fail on the fix(ci): make the weekly performance gate tolerate runner and network noise #337 version.tests/test_project_consistency.py: the baseline-promotion check now finds the regression step by itspython scripts/compare_performance.pycall, not its display name.performance.yml: the step label states the new rule.CHANGELOG.md: fix(ci): make the weekly performance gate tolerate runner and network noise #337's Unreleased entry is rewritten to describe the final behaviour.Verification
ruff check,ruff format --check,mypyand the pre-commit hooks are clean.gh workflow run performance.ymlonce. Expect a pass, with time warnings forapps/recommend/outdatedif the runner is slow.🤖 Generated with Claude Code
Summary by Sourcery
Make the performance gate distinguish actionable memory and severe runtime failures from wall-clock noise on shared CI runners.
Bug Fixes:
Enhancements:
Tests: