Repository navigation
fix(breakdown): record the GEAK candidate at close and kernel e2e gain from the rebench - #2048
Conversation
…ppened Enablement counted a KEEP awaiting revalidation as landed, close never wrote the GEAK candidate it later rendered warnings from, the dropped-status set named a status nothing writes, a GEAK attempt's e2e gain carried GEAK's own figure instead of the rebench measurement, and a forge kernel's e2e gain was the best of all gates while its decision was the last one.
Code metrics gate: PASSEDJudged: the 15 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
Module length warning: over 800 lines, within the 1200-line failure limit (not failing): 1
Already over at the base (backlog, not this change's): 1
107 findings in files this change does not touchOutside this change (informational): 107
...and 82 more; the job log lists every row.
Thresholds and sources: |
…n the wiring test
…a stale GEAK candidate on resume Landed and validation-pending counts are assembled from the revalidation windows instead of the KEEP row flags, which are written once. An empty GEAK slot is recorded as null so a resumed close replaces the earlier block.
44695c8 to
dff890f
Compare
Restore the enablement attempt counts and their rendering to the base behavior; a revalidation window does not name its KEEP, so the counts need an orchestrator-side record instead of an assembly-time pairing.
lishuoshuo-amd
changed the title
…iting GEAK candidate Under rebench-first an awaiting candidate is never part of the validated gain, so it adds only its own AWAITING fact and warning and the validated gain renders as recorded.
…n branch A pending candidate is never in the final stack under rebench-first, so the unreachable provisional scenario is dropped and the awaiting case asserts the exact facts and warnings.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "GEAK candidate (self-reported +13.79%) is AWAITING a main-flow rebench — excluded from the headline gain " | ||
| "and final stack until a measured rebench validates it.", |
PR #2048 -- fix(breakdown): record the GEAK candidate at close and kernel e2e gain from the rebenchWhat it does: This PR fixes two places where the breakdown recorded the wrong value or no value. CLOSE now writes the unsettled GEAK candidate slot to Blocking issues: none SKIPPED: V4 -- the "# PARTIAL:" lines in openprs.txt mean overlap with those open PRs (each was compared on its first 100 changed files only, and only the newest 100 open PRs were compared) is unchecked. Checked:
Ran:
Independent reader: second agent (see independent.txt) | Base: c565c19 | Head: d54ce67 LGTM |
The roofline, baseline and GEAK candidate records each run as a separate close step, so a failure is attributed to the record that failed and one failing record no longer skips another.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| "GEAK candidate (self-reported +13.79%) is AWAITING a main-flow rebench — excluded from the headline gain " | ||
| "and final stack until a measured rebench validates it.", |
PR #2048 -- fix(breakdown): record the GEAK candidate at close and kernel e2e gain from the rebenchWhat it does: CLOSE now records the unsettled GEAK candidate slot as its own Blocking issues: none SKIPPED: V4 -- the "# PARTIAL:" lines in openprs.txt mean overlap with those open PRs (each was compared on its first 100 changed files only, and only the newest 100 open PRs were compared) is unchecked. Checked: close.py under main's async structure: LGTM |
PR #2048 -- fix(breakdown): record the GEAK candidate at close and kernel e2e gain from the rebenchWhat it does: CLOSE now records the unsettled GEAK candidate slot as its own Blocking issues: none SKIPPED: V4 -- the "# PARTIAL:" lines in openprs.txt mean overlap with those open PRs (each was compared on its first 100 changed files only, and only the newest 100 open PRs were compared) is unchecked. Checked:
Ran:
Independent reader: second agent (see independent.txt) | Base: 9f7030a | Head: 5df30c0 LGTM |
haishuok0525
left a comment
haishuok0525
left a comment
There was a problem hiding this comment.
What it does:
- CLOSE now records
shared_state.geak_pendingas its owngeak_candidateclose step viaclose_out.record_geak_candidate, writingnullfor an empty slot so a resumed session's stale block is replaced. The dropped-status set becomes one constant,kernel_records.GEAK_DROPPED_STATUSES = {rebench_cancelled, rebench_declined, overlay_unloadable}, used by both the Final renderer and the mission summary. The unreachable provisional / cross-harness branches infinal.renderare removed. - A GEAK attempt's
e2e.e2e_gain_pctis now Hyperloom-measured: null at record time and stamped from the rebenchdelta_pctin_settle. GEAK's own figure moves toself_reported_e2e_gain_pct. On the forge route,_integrate_e2etakes the gain from the same (last) row as the decision instead of the maximum across all gates.
Blocking issues: none.
Checked:
- Status vocabulary:
rebench_unavailablehas no writer anywhere insrc/.rebench_cancelled(kernel.py, cancelled explore rebench) andrebench_declined/overlay_unloadable(_geak_decline_statusin_revalidate_on_geak_harness) are exactly the statuses written when no verdict is reached. - Field names: the
geak_pendingkeys read byrecord_geak_candidate(self_reported_gain_pct/_tput/_basis,revalidation_error) match_record_geak_candidateand the decline/cancel writers. - Close ordering: the snapshot runs after
stack_revalidationandpost_opt_roofline, and it runs inside_run_close_step. The failure isolation frombaseline_progressis covered by a test. - Settlement:
_settleruns at assembly, so stamping is idempotent across exports.reject_geak_attemptsno longer copies the (now null)e2e_gain_pctover the claim. The KB / remote-recipee2e_gain_pctvalues come from integrate records and stack entries, not from this block. The_kernels/ capability counters accept null. - Final renderer: dropping
headline_unvalidatedcannot surface a fabricated0.00%, becauseoutcome.final.gain_pctisvalidated_total_gain_pctand stays null without a validation. - The description matches the diff, and CI is green.
LGTM
.claude/skills/review-pr/evals/cases.yaml holds 11 cases, each a PR at a head a reviewer once saw and what a review of that head must report: - 6 rules.md source PRs, at the head the finding was raised against and with the place it was raised at: D7 #1567, S1 #1564, P1 #1526, R4 #1563, X6 #1497 and M3 #1444. Each finding was fixed afterwards. - #2045 at ab239fd, where two cards were clean and a third found the C2 the author then reproduced and fixed. - 4 clean cases, #2048, #2044, #2039 and #2059: every card on them was clean, the head they reviewed is the head that merged, and no later commit on main names them. A finding about the description is left out, since a replay reads today's description, which the author may have corrected since; so is #2094, whose blocking card came after the merge and was never settled. scripts/review_pr_eval.py score reads one eval-<case>-r<repeat> directory per replay and reports how many expected findings were found (by key, or by a cited place within ten lines), how many clean replays blocked, whether the replays of one case agree, and the time, turns and tokens from the run log. Replayed through fetch.sh, #1567 at a143f8f and #1444 at 6767a77 cut their comments before the finding was raised and carry the expected file in their diff. Co-authored-by: Cursor <cursoragent@cursor.com>
Two recorder facts were written from the wrong source or never written, so the breakdown and the report stated outcomes that did not happen. Enablement landed counting is left for a separate change: a revalidation window does not name its KEEP, and resume recovery can end a window without an outcome row.
close.geak_candidatehad no writer, so the Final section's GEAK candidate AWAITING / DROPPED warnings could never fire. CLOSE now recordsshared_state.geak_pendingas its owngeak_candidateclose step (a failure in it, or in the baseline record, is attributed to and isolated from the other) (non-empty only when a candidate is still awaiting a rebench or was dropped without one) through a newclose_out.record_geak_candidate, withrevalidation_pending = (status == "awaiting_rebench"). An empty slot is written asnull, which overwrites the block an earlier CLOSE of a resumed session left (the singleton merge replaces a key with a non-mapping value), so a candidate settled after resume no longer renders a stale warning. The dropped-status set read by the Final renderer and the mission summary namedrebench_unavailable, which nothing writes, and missedrebench_declined/overlay_unloadable, which the GEAK-harness fallback writes when it reaches no verdict. It is now one constant,kernel_records.GEAK_DROPPED_STATUSES = {rebench_cancelled, rebench_declined, overlay_unloadable}, used by both readers. The Final section's provisional / cross-harness branches, unreachable before this writer and premised on a candidate counted in the headline, are removed: an awaiting candidate adds only its AWAITING line.e2e_gain_pct:e2e.e2e_gain_pctcarried GEAK's self-reported figure (glm52add_rmsnorm_quant: 40.80%) while Hyperloom's rebench measured 10.71%. GEAK's figure now goes toe2e.self_reported_e2e_gain_pct, ande2e_gain_pctis set from the rebench delta when the attempt settles (newrecorder/kernel_gain.py), null until then. A revoked KEEP keeps its self-reported figure instead of overwriting it._integrate_e2etook the best gain over all gates whiledecisionwas the last gate's. The gain now comes from the same (last) row as the decision, null when that row measured nothing; each gate's figure stays on its integrate row.main(fix(breakdown): rename throughput_tok_s_per_gpu to output_throughput (schema 6.1) and record per-GPU output on baseline and validation rows #2023 has merged).test_close_phase_sequencer.py(CLOSE write ->collect_v6_close-> Final render shows AWAITING / DROPPED for each dropped status; empty slot clears; a two-leg realon_enter_closeshows no stale warning),test_geak_decline_status.py(mission summary for each dropped status),test_geak_gain_alignment.py(test_report_shows_provisional_not_zero_validatedremoved: its scenario, a pending candidate already in the finalaction_pathwith a higher final throughput, cannot occur under rebench-first, wheregeak_e2eenters the stack only when the rebench promotes it;test_report_shows_pending_candidate_excluded_from_headlinenow asserts the exact facts and warnings),test_geak_revalidation_dispatch.py(an awaiting candidate with validated gain -1.5 / 0 / none / 3.0 yields exactly the validated-gain lines plus its AWAITING fact and warning),test_sbd_v6_kernel_timeline.py(full e2e block with measured 10.7132 vs self-reported 40.799, no measurement -> null, revoke keeps self-reported, forge takes the last gate, accepted GEAK candidate with no rebench verdict -> null gain, empty delivered gain andno_improvement); they fail on the base branch except four that pin behaviour base already had:test_a_revoked_geak_keep_keeps_the_claim_it_made,test_close_sequencer_records_no_geak_candidate_when_none_is_pending,test_mission_summary_flags_a_dropped_candidate[rebench_cancelled]and the positive-gain case oftest_an_awaiting_geak_candidate_adds_only_its_own_lines.test_geak_promotion_retention.pynow expectse2e_gain_pct is Nonewith the self-reported 50.0. Ran these and the related close / geak / reporter test files individually;ruff,lint-importsandscripts/code_metrics.pypass. Replayed glm52 (e2e_gain_pct10.7132,self_reported_e2e_gain_pct40.799) through the recorders.kernel_gain.py, kept out ofkernel_event.pybecause that module is at its line budget), 6 test files, andcode_metrics_baseline.txt, which lowers four entries the change shortened.overlay_unloadable) now hasattempts[].gain_pct = nullandoutcome.delivered[].gain_pct = null, and the visitoutcome.verdictisno_improvement("nothing measured a gain on the candidates kept") instead ofimprovedwith GEAK's own figure: nothing Hyperloom measured backs the gain.attempts[].gain_pct,outcome.delivered[].gain_pctandoutcome.verdictchange as described under Observable effect; kernele2e.e2e_gain_pctchanges value semantics;close.geak_candidateande2e.self_reported_e2e_gain_pctare new or newly populated fields.