Skip to content

Stop main benchmarks once three of four candidates agree - #1130

Merged
helly25 merged 12 commits into
mainfrom
fix/main-benchmark-fourth-agreement
Nov 7, 2026
Merged

helly25 merged 12 commits into
mainfrom
fix/main-benchmark-fourth-agreement

Conversation

@helly25

@helly25 helly25 commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Stop main-branch benchmark sampling as soon as a fourth candidate supplies the required compatible quorum. Main previously ran a fifth candidate unconditionally; the recorded extra macOS job took 22m29s even though the final aggregate selected candidates 0/2/3.

AG;DR

Concrete failure and behavior

  • Main benchmark run 42330247443 completed successfully, but its fifth macOS measurement was unnecessary. The retained aggregate selects candidates 0/2/3 with identical macOS 26.7.2, three-CPU and binary/build contracts, pooling nine rounds and retaining seven. The fifth job 128298925253 still ran 04:19:13–04:41:42 UTC. This is observed job elapsed time, not a promise of equal future critical-path savings.
  • The PR workflow already makes an after-fourth decision. Add the missing equivalent select-fourth job to .github/workflows/benchmarks.yml, downloading completed candidate artifacts and calling the existing --decision-stage=fourth logic. Do not add another decision job when the first three already agree.
  • Gate each platform's fifth measurement on the corresponding decision output and selector success. Give optional fourth/fifth jobs explicit cancellation/status guards so a skipped sibling cannot activate GitHub's implicit success() suppression. The final aggregate explicitly waits for the new selector and all candidate jobs.
  • Preserve the three-compatible-candidate requirement, CPU allocation, source/binary/tool identity checks, OS fallback policy, raw observations, estimator, compiled artifacts and normal failure propagation. No benchmark algorithm or application C++ changes.

Regression checks and documentation

  • Extend the existing optional-job guard regression to both main and PR workflows, and check that both wire fourth-stage decision outputs into fifth-job gating and final aggregation. Existing tests retain agreed-three, agreed-four, unresolved 2+2, contract/tool mismatch, exact-OS preference, fallback and provenance coverage.
  • The new workflow checks produce five expected failures against the exact published baseline and pass with this correction. All 31 shard tests pass with Python 3.13. Actual bazel test //tools:benchmark_shards_test --test_output=errors passes with declared workflow runfiles; it executed the target after building its existing native helper. No new runfile/dependency is needed.
  • Update the legacy benchmark-history expectation to require the conditional fifth-stage decision instead of the old unconditional schedule. Both workflow test suites are part of the validation below.
  • Update the canonical TODO with the measured wasted-work evidence and add a concise CHANGELOG entry. Do not change the existing 1127–1129 heads or fold this CI correction into their qualified source.

Validation, scope and dependencies

Normal signed commit bf2eddd53001ce189cf326207a539916b9991a13 and normal push succeeded. Configured policy hooks, actionlint, Trunk formatting/checks, canonical Markdown formatting and diff checks pass, with all four published file hashes unchanged. No local clang-tidy executable ran. The first direct full test attempt used the system Python 3.9 and failed on existing unsupported zip(strict=True) calls; that environment result is retained separately, not fixed by weakening production code. The supported interpreter and Bazel runs above pass.

This separate five-file successor depends on #1129 (62030d1d3608817db2f0b7c66a961bc7167b0435), following #1127/#1128. Preserve each ready parent and merge separately; require this PR's own current-context CI and normal post-retarget main validation. The new main-only job is included in that workflow's final aggregate dependencies; the PR workflow and its done.needs job set are unchanged. First post-merge main automation still needs inspection; private/local tests do not substitute for required checks or an actual deployed run.

The ancestry was synchronized and pushed through normal signed merges after CI found three narrow evaluate.cc lint issues in #1127 and the required Brotli byte-pointer cast annotation in #1128. That synchronization preserved this PR's own implementation/evidence payload. The corrected current head requires its own full CI; successful ancestor or previous-head checks are not substituted. Healthy reachable collectors were not cancelled, and local clang-tidy was not run.

The first corrected-ancestry coverage run exposed an older benchmark_history_test that explicitly required the previous unconditional fifth run. This is a stale workflow expectation, not a C++ failure or a coverage-percentage failure. The test now requires a successful select-fourth decision and the platform's explicit *_fifth == 'true' output, and forbids the old initial-selector-only gating. Both relevant Python suites (benchmark_history_test and benchmark_shards_test) passed all 62 tests from an isolated exact-source Linux validation directory; workflow/test SHA receipts and the original failure log are retained. The earlier 31-test run covered only the shard suite; this correction adds the existing history-suite contract to validation. Normal publication hooks and fresh own-head CI remain required.

The stale-test correction is published as normal signed commit 3a464cdce39197a6cb1a4819f7809948abc46198. Scoped Trunk formatting and all configured normal commit/push hooks passed; formatter output preserved the exact test/workflow SHA values validated by the 62-test Linux run. No local clang-tidy ran. Healthy ancestor heads and CI collectors were preserved. Fresh CI on this final head remains required.

Current main synchronization: after #1129 merged as 77aa9ef9bc9475adcbdd37cf3004143a96f03060, the normal signed merge preserves the entire previous child tree byte-for-byte. The actual included parent 62030d1d is an ancestor; its full tree, qualified parent 73de2769, and the main squash tree are identical. Normal merge/push hooks passed; fresh own main-context CI is required and earlier stacked checks are not substituted.

Additional live evidence: unchanged main run 42349583897 queued a fifth Mac candidate even though the canonical fourth-stage selector, run against all seven completed candidate artifacts, reports linux_fifth=false and macos_fifth=false. The receipt and original artifacts were retained locally; no CI was canceled and this observation does not replace this PR's checks.

@helly25 helly25 changed the title Stop main benchmark sampling after a four-candidate quorum Stop main benchmarks once three of four candidates agree Nov 7, 2026
@helly25
helly25 merged commit ae8c5af into main Nov 7, 2026
32 checks passed
@helly25
helly25 deleted the fix/main-benchmark-fourth-agreement branch November 7, 2026 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant