fix(explore): report the decision round, not the post-KEEP confirmation one - #1334
Open
zengleixin-amd wants to merge 3 commits into
Open
fix(explore): report the decision round, not the post-KEEP confirmation one#1334zengleixin-amd wants to merge 3 commits into
zengleixin-amd wants to merge 3 commits into
Conversation
…on one A search explore ran three rounds on one server: a discarded cold warmup, the decision round that graded the variant, and -- on KEEP -- a third confirmation round. The third round measured a server the first two had already warmed, so it carried more cache than the decision round whose number it then overwrote as the headline. That inflated value also became the anchor the next in-batch variant was graded against, so the bias rode the KEEP chain rather than staying with one variant. Search explore now stops at the decision round. The flag is set where explore params are built for a search -- the proposal injector, and the multi-node auto-materialise path that builds its own -- so internal revalidation tasks, which construct params directly and never reach the injector, keep the confirmation round they rely on. GEAK's same-harness revalidation is one of those. cumulative_gain_validated now records e2e_decision_round as its basis so a reader can tell an explore number from a revalidated one.
The suite proved the confirmation round runs and proved the flag reaches the executor, but nothing proved what the run looks like once it is off. The fake hands the confirmation slot a throughput far above the decision round's, so a round that still ran would show up in the ledger rather than only in a call count.
…stale rebench prose Review follow-ups on this change: The basis was a literal, so a caller that asked for the confirmation round (the flag is a setdefault) still recorded e2e_decision_round. Read it from the winner instead of from the request: a variant carries stack_rebench_tput only when a confirmation round actually ran, which is what the label describes. The injector comment claimed every revalidation task keeps the confirmation round. Only GEAK's does, by omitting the key; the resume full-stack one sets False itself. Three LLM- and report-visible strings still said explore inlines a per-KEEP rebench. They are outside this change's own files but this change is what made them false.
CI E2E report — ❌ Timeout
|
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.
A search
exploremeasured a variant over three rounds on one server: adiscarded cold warmup, the decision round that graded it, and — only on KEEP —
a third confirmation round. The third round measured a server the first two had
already warmed, so it carried more cache than the decision round whose number
it then overwrote as the headline (
explore.py, KEEP branch). That inflatedvalue also became
running_base_tput, the anchor the next in-batch variant wasgraded against, so the bias rode the KEEP chain instead of staying with one
variant.
A search now stops at the decision round: the round that grades the variant is
the round that gets reported.
What changes
enable_stack_rebenchalready existed and the executor already had thetwo-round path behind it (
explore.py, theelsearm of the KEEP branch).This flips who asks for it, at the two places explore params are built for a
search:
loop/proposals.py—_inject_explore_runtime_params, the single source oftruth for the propose/Critic and direct-delegate paths.
phases/explore.py— the multi-node auto-materialise grid, which builds itsown params and never reaches that injector. Without this line, single-node
and multi-node searches would report differently.
Internal revalidation tasks construct params directly and never reach the
injector, so they keep the confirmation round they rely on:
writeback.py)Truewriteback.py)Falseintegrate_patchintegrate_handler+BaselineExecutorintegrate_patchis deliberately left alone. Its confirmation round boots itsown server and warms it, so it is not a hotter sample of the main bench's
server — and it re-runs the accuracy gate, which is worth more there because a
failure means a patched source tree to roll back.
cumulative_gain_validatednow recordse2e_decision_roundas itsmeasurement basis for this path, so a reader can tell a search number from a
revalidated one instead of both arriving as
e2e_rebench.Prompt text that told the orchestration LLM every KEEP triggers an inlined
rebench, and that exposed
rebench_stable_threshold_pcton the explore paramssurface, is updated — the flag is now off there, so the knob was dead.
Trade-off
The confirmation round was also the noise gate: it could turn a KEEP into
KEEP_UNSTABLEand evict it. Nothing replaces that at the variant level, so aone-off high sample now enters
optimization_stackon the decision roundalone. The session-level net is unchanged — the
cumulative_gain_validated_stack_lenwatermark and the resume/GEAK full-stackrevalidations still run. This is deliberate: the gate was reading the most
cache-warm sample in the run, so it was admitting on a number it had inflated
itself.
KEEP_UNSTABLEand thestack_rebench_*fields stop being produced on thispath. Every consumer reads them through
.get()or an allowlist membershiptest, and
writeback.pyalready collapsedKEEP_UNSTABLEintoOUTCOME_REVERT, so nothing downstream breaks on their absence.Validation
Full suite on Linux, this branch vs
main(2a11ff6a8), same command:Failure sets compared item by item — identical;
ONLY-IN-PATCHED: 0. The 58are pre-existing on
main. The +3 are the tests below.test_a_search_keep_stops_at_the_decision_roundis the one that pins thebehaviour: it feeds the confirmation slot a throughput far above the decision
round's, so a round that still ran would show up in the ledger rather than only
in a call count. It asserts two Magpie runs, no confirmation slot, and a
headline taken from the decision round.
test_explore_executor_defaults_to_warm_decision_matching_hot_baselinestillasserts three rounds when the flag is on, so the capability the internal
revalidation paths depend on stays covered.
Also added: the injector sets
False; an explicit caller still wins over it(
setdefault); the multi-node grid carries it.Not covered by tests: whether the reported number actually moves on real
hardware.
run_grid's auto-warmup is disabled under pytest, so the cacheeffect this addresses cannot be reproduced in the suite — only the question of
which round becomes the headline can.