[FIX] make the grading anchor and the config it was measured on inseparable - #1091
Open
ZhengGong-amd wants to merge 11 commits into
Open
[FIX] make the grading anchor and the config it was measured on inseparable#1091ZhengGong-amd wants to merge 11 commits into
ZhengGong-amd wants to merge 11 commits into
Conversation
baseline_hot_tput was assigned float(tput) in the same branch that assigned baseline_tput the identical value, so the two fields were provably equal and the mirror carried no information. Nothing in the runtime ever read it; only tests asserted on it. baseline_cold_tput stays: it is the only durable record of the discarded cold round, since current_best.cold_tput is overwritten by the first KEEP. Co-authored-by: Cursor <cursoragent@cursor.com>
…sion round Only last_run_tput's None-ness was ever read, and every path that appended to winners assigned it first, so the guard it gated could not fire. Fold it into the plain `winners` check. cold_tput held the round-1 measurement, which is warm whenever warm-decision is active, so the name asserted the opposite of what it carried. Rename it (and its ledger key) to decision_tput, leaving cold_tput to mean the genuinely cold round the baseline double-run discards. Co-authored-by: Cursor <cursoragent@cursor.com>
…he env opt-out INFERENCE_OPTIMIZER_EXPLORE_WARM_DECISION could turn off the warm decision round on its own, while the baseline deliberately exposes no control over its cold+hot double-run for exactly this reason. Setting it therefore graded cold candidates against a hot anchor -- a systematic negative bias across a whole round, with nothing reporting it. Both sides already key off the same resolve_lifecycle_params verdict, so they measure hot together or cold together. Keep that coupling and remove the only way to break it. lifecycle_eligible stays: it is a capability gate, not redundancy -- scriptable frameworks boot no server, server_lifecycle is local-only, the reuse protocol lives in the Magpie built-in scripts, and torch_profiler is incompatible. Tests that need the single-round cold path now state that precondition directly instead of flipping the env var. Co-authored-by: Cursor <cursoragent@cursor.com>
…e number base_tput and base_extra_args are seeded side by side from the same current_best when the proposal is materialized, so they are a matched pair by construction. The executor-side drift guard refreshed only the anchor, so once a KEEP landed while the task sat queued, a candidate was launched on the snapshotted stack and graded against the newer number -- a variant that was merely neutral then read as a regression. Add resolve_grading_base(), which hands back the anchor together with the args, envs and arg-mode controls it was measured on, and have explore replace the whole triple or none of it. It returns None when current_best carries no positive throughput: the anchor is then baseline_tput, whose args live in the baseline record rather than on state, so the dispatch-time snapshot stays authoritative. That case keeps its own anchor-only recovery branch, without which a params omission grades every variant against zero. Co-authored-by: Cursor <cursoragent@cursor.com>
…aded on The round-1 KEEP decision compares a variant against running_base_tput, which advances with every KEEP in the batch, but the round-2 confirmation floor was built from the round-start base_tput. For the 2nd and later variants the floor therefore sat a whole KEEP below the live bar, so a variant that regressed against the very recipe it was layered on still cleared "stable", had its gain recomputed into a negative number, and was KEPT -- dragging running_base_tput down and lowering the bar for everything after it. Anchor the floor and the enable check on running_base_tput so confirmation and decision agree. The added test reproduces the old outcome: with the previous floor the regressing variant lands as KEEP with a negative gain_pct instead of KEEP_UNSTABLE. Co-authored-by: Cursor <cursoragent@cursor.com>
Those commits were meant to delete redundant throughput state, but added more than they removed. Reverse that: - Drop resolve_grading_base(). It was a near-duplicate of resolve_grading_anchor_tput plus _first_positive_tput whose only work was copying current_best fields into a dict, so explore now reads current_best directly and no new public API is introduced. - Drop the base_tput <= 0 fallback branch. live_anchor > base_tput already holds when base_tput is 0, so the branch could never be the deciding one. - Delete baseline_cold_tput, which no runtime code reads, along with the warmup_anchor if/else that existed only to write it -- both arms assigned baseline_tput the same value. The cold round is still recorded on current_best["cold_tput"]. - Cut the comments and docstrings back to the constraint being stated instead of narrating why the change is correct. No behaviour change: the grading anchor and its stack are still refreshed as a pair, and the rebench floor still follows the in-batch anchor. Co-authored-by: Cursor <cursoragent@cursor.com>
The anchor a candidate is graded against and the args/envs it is launched on top of are one unit, but seeding them was open-coded at seven call sites with four different shapes: setdefault-everything in proposals, write-if-non-empty in the phases, and an overwrite rebind in integrate_patch. Nothing held them together, so a site could seed one half and silently omit the other. Project the base_* params from current_best in one place instead, keyed on the fields current_best actually carries. anchor= opts into base_tput so a grading caller takes both halves from the same snapshot; overwrite= is for an execution-time rebind, and is also what lets an empty value clear a superseded layer rather than read as "no config". No call site is rewired yet. Co-authored-by: Cursor <cursoragent@cursor.com>
Replaces the six open-coded copies -- four action branches in _materialize_approved_proposal plus the mn-auto-materialize, framework config exploration, internal sweep, internal analysis and integrate_patch rebind sites -- with inject_stack_base_params, deleting ~130 lines including two byte-identical blocks in phases/explore.py and phases/framework.py. Three inconsistencies go with them: profile, sweep and integrate_patch seeded args but not envs where explore seeded both; the phases hand-rolled the list-control coercion that to_str_list already does; and the integrate_patch rebind overwrote the anchor and args while only filling absent controls, so a superseded layer's controls survived the rebind. Co-authored-by: Cursor <cursoragent@cursor.com>
_route_delegate claimed parity with _materialize_approved_proposal but injected only base_tput, so an LLM-driven delegate explore launched every variant on a bare config while grading it against the established recipe. A 24h gemma-4-26B run shows the cost: with the anchor at 7725 tok/s, six unrelated variants across two rounds all measured 4577-4882 -- the raw baseline, within 0.7% for five of them -- and were recorded at -37% to -41%. The rounds read as "every lever regresses" when the levers had never been layered on anything. The executor-side drift guard cannot cover this. It fires on live_anchor > base_tput, and both sides come from resolve_grading_anchor_tput, so a correctly seeded anchor with missing args compares equal -- exactly the case that needs the re-read is the one it skips. Seed both halves through inject_stack_base_params. Co-authored-by: Cursor <cursoragent@cursor.com>
…lers' docs
Self-review of the preceding three commits:
- Fold the two single-use normalizers into _STACK_BASE_FIELDS. Naming two of the
five and inlining the third was inconsistent, and the table is the only reader.
- Type the normalizer slot as Callable[[Any], Any] rather than Any.
- Drop the isinstance guard on extra_envs for the (v or {}) idiom the seeding
sites already used; the guard was defensive, not load-bearing, and it was what
pushed that entry to 119 columns.
- _materialize_approved_proposal still advertised "grid executors get current
best tput as base_tput", which has understated the contract since the anchor
and its config became one unit.
- Restate the delegate-path comment as the invariant it guards. "Parity with
_materialize_approved_proposal: direct delegates need the same knobs" named no
knobs, and was already there while the parity it asserted did not hold.
- Cut the remaining comments to the constraint, dropping a stale parenthetical
about resolution order at the integrate_patch rebind.
Co-authored-by: Cursor <cursoragent@cursor.com>
The executor's anchor-drift rebind hand-copied the same five current_best fields inject_stack_base_params already normalizes, so _STACK_BASE_FIELDS had two readers and a field added to one would silently miss the other. Extract stack_base_params(current_best) as the single projection and have both use it. The executor now rebinds params before reading its locals out of them, which leaves one read path instead of a read followed by a conditional re-assignment of every local. params is a shallow copy of task.params, so the in-place update never reaches the persisted row. One edge changes: a current_best carrying positive throughput but no extra_server_args key no longer clears the params snapshot, because the projection omits absent fields rather than defaulting them. Only current_best records written by _lift_to_current_best reach that branch and those always carry the key, so the case is unreachable in practice; preserving the snapshot is the safer reading of "the field says nothing". Co-authored-by: Cursor <cursoragent@cursor.com>
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
1 participant
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.
Branch:
fix/zgong/baseline-tput, 10 commits on top ofa56bf465a.A gain only means something when both numbers were taken on the same
configuration.
05a1fd6e5centralised the number inresolve_grading_anchor_tput(); this branch makes the number and the configtravel together and removes the ways they came apart. Evidence is session
96925(gemma-4-26B-A4B-it, vLLM/MI355X,code_revision 16eb8c4): reported58.93% gain after measuring 65.65%, with six consecutive −37% to −41% EXPLORE
results from levers that never ran.
Fixes
The delegate path seeded the anchor and nothing else —
b9f0711e7._route_delegateclaimed "parity with_materialize_approved_proposal" butinjected only
base_tput, omittingbase_extra_args,base_extra_envs,base_remove_args,base_unset_envs,base_args_mode. Variants thereforelaunched on a bare config while being graded against the established recipe. All
five EXPLORE rounds in
96925took that channel (base_extra_argsabsent as akey in every
tasksrow,base_tputpresent and correct); with the anchor at7725.60 tok/s and baseline at 4663.79:
kernarg-hipblasltaiter-rmsnorm-onmoe-dispatch-policy-1block-size-32max-num-seqs-128max-batched-tokens-32768Six unrelated levers clustering on the raw baseline is the naked-config
signature, not six bad levers; grading the bare baseline against that anchor
gives −39.63%.
The executor's drift guard cannot catch this. It fires on
live_anchor > base_tputand both sides resolve throughresolve_grading_anchor_tput, so when the anchor is seeded correctly and onlythe args are dropped the two are equal by construction — a strict
>skipsexactly the case that needs it. It has to be fixed at the source.
Confirmation disagreed with the decision —
4aa023381. Round 1 gradesagainst
running_base_tput, which advances with each in-batch KEEP, but theround-2 stability floor was built from the round-start
base_tput. For the 2nd+variant the floor sat a whole KEEP below the live bar, so a variant that
regressed against the recipe it was layered on still cleared
stable, had itsgain recomputed negative, and was KEPT — lowering the bar for everything after.
Floor and
enable_stack_rebenchnow anchor onrunning_base_tput.A cold candidate against a hot anchor —
febe75fc2.INFERENCE_OPTIMIZER_EXPLORE_WARM_DECISIONcould disable the warm decision roundwhile
baseline_tputstays the hot round of the baseline double-run, biasing awhole round negative. Both sides key off the same
resolve_lifecycle_paramsverdict; the env var is gone so that coupling is the only control.
Paired re-read —
8963524bd. The queued-snapshot supersede refreshed onlythe anchor; it now takes args, envs and arg-mode controls from the same
current_best, all or none.Refactors
bbeccf745+7ed4eea44+7fb6119fe: the fivebase_*params were seeded byhand in six functions across six files in five different shapes, and not at all
on the delegate path.
inject_stack_base_params(params, state, *, anchor, overwrite)instate/shared_state.pynow owns the projection (15 insertions,140 deletions). It also settles three inconsistencies:
profile/sweep/integrate_patchseeded args but not envs; the phases hand-rolled whatto_str_listdoes; theintegrate_patchrebind overwrote anchor and args butonly filled absent controls.
0417d6ebf+bfb29bc0e: dropbaseline_hot_tput(provably equal tobaseline_tput) andbaseline_cold_tput(no runtime reader); rename the exploreledger's
cold_tputtodecision_tput, since it holds the round-1 measurementwhich is warm whenever warm-decision is active; delete the
last_run_tputguardthat could not fire.
fd4b0b437+7fb6119feare self-review passes reversing redundancy thepreceding commits added.
Commits
0417d6ebfbfb29bc0efebe75fc28963524bd4aa023381fd4b0b437bbeccf7457ed4eea44b9f0711e77fb6119feTests
test_critic_verdict_map.py::test_delegate_explore_seeds_the_stack_with_the_anchor— verified by reverting
_route_delegate, which reproduces the sessionsignature:
KeyError: 'base_extra_args'.test_explore_executor.py— paired re-read, and the rebench floor followingthe in-batch anchor (old floor lands the regressing variant as
KEEPwith anegative
gain_pct).test_shared_state_units.py::TestInjectStackBaseParams— 7 cases: anchoron/off,
setdefaultvsoverwrite, clearing a superseded layer, fieldscurrent_bestlacks,baseline_tputfallback,Nonetolerance.resolve_lifecycle_paramsinstead of setting theremoved env var.
717 passed / 1 skipped across the 17 affected test files.
ruff checkandpylint --errors-onlyshow no new findings vs. the pre-change baseline.Breaking changes
None.
SharedState.from_dictfilters to known__dataclass_fields__, so an oldstate.jsoncarrying the two removed fields still loads.cold_tputis adiagnostic ledger key with no runtime reader;
current_best["cold_tput"], whichis read, is untouched. The removed env var appeared in no doc, script or workflow.
Not in this branch
executors/sweep.py::_build_gridbuilds
variant_envsfrom scratch and never mergesbase_extra_envs, so thesweep runs the champion's args with none of its envs — in
96925the finalsweep ran without the six AITER variables. This branch makes the fix a one-line
merge, but it changes what the sweep measures, so it needs its own before/after.
writeback.py::_enqueue_internal_stack_rebenchkeeps hand-rolled seeding:revalidation puts the stack in the grid variant, so the helper would
double-apply it.