Repository navigation
fix(report): count dispatches, kernel outcomes and critic passes once each - #2045
Conversation
The session report mixed axes and scopes in several places: the headline paired output tok/s with a p50 gain, elapsed showed one leg, the stop target was printed as the objective, a zero TTFT or kernel speedup read as a measurement, the final unit did not match its value, an unvalidated session got a contradictory validation warning, failed roofline runs read as no data, and the LLM prompt keyed not-validated on a value never emitted.
A re-anchored baseline no longer matches the ledger baseline the gain is measured against, so the pair is dropped unless before*(1+gain) equals after within 0.5%. The graded axis no longer falls back to the configured axis, which is recorded as p90 while the verdict is taken on p50.
Before and after now come from the last stack event settled validation row, the one that produced the gain, and only when it is on the graded axis and reproduces the gain. Otherwise the headline states the gain and the axis without throughput figures.
Specialist rounds counted a discovery/authoring dispatch twice (phase row and run share one id), the kernel table put one attempt in both reverts and rejected and took capability gains from reverted candidates, the funnel called lane-tried kernels optimized, and the critic table ordered and numbered passes by a per-process counter that restarts on resume.
Code metrics gate: PASSEDJudged: the 14 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
108 findings in files this change does not touchOutside this change (informational): 108
...and 83 more; the job log lists every row.
Thresholds and sources: |
The critic report renderer imported the placeholder from the agents layer, which inference_optimizer may not depend on. The constant now lives in the intent protocol module that both sides already import.
PR #2045 -- fix(report): count dispatches, kernel outcomes and critic passes once eachWhat it does: The session report counted record views rather than the things they describe: a specialist dispatch recorded both as a phase action row and as a framework run with the same id was counted twice, one kernel attempt landed in both a decision bucket and an outcome bucket, and critic passes were ordered by Blocking issues: none SKIPPED: V4 -- openprs.txt compared only the first 100 files of #1032, #1154, #1276, #1869, #1907 and #2004; overlap there is unchecked. Checked: specialist_tally, source_counters, _kernel_funnel, compose funnel lines, critic renderer, _compose_critic, protocol/intent.py, intent_envelope.py, recorder/kernel_event.py::_settle and ::_merge_acceptances, framework_event.py::record_run, critic_out.py::record_critic_iteration, docs/reference/session-breakdown.md (critic section, compatibility rules, stability guarantee), AGENTS.md Clean design / Finish the replacement; sweep scoped to src/hyperloom/inference_optimizer, src/hyperloom/agents/critic and docs for every reader of kernel_pipeline_funnel, source_counters, specialist_tally, critic.iterations and DEFAULT_IDLE_BODY; outcome x e2e.decision combinations and task_id/run_id overlap surveyed across 9 real sessions | Ran: the six changed test files plus test_intent_envelope.py at head (144 passed); the same test files against a git-archived base (11 failed: every new or changed test); real-session reports rendered at head and base (rounds 91/104/31/18/61/27/38 -> 88/100/27/13/56/24/32, glm52 LGTM |
PR #2045 -- fix(report): count dispatches, kernel outcomes and critic passes once eachWhat it does: The session report counted record views rather than the things they describe: a specialist dispatch recorded both as a phase action row and as a framework run with the same id was counted twice, one kernel attempt landed in both a decision bucket and an outcome bucket, and critic passes were ordered by Blocking issues: none SKIPPED: V4 -- openprs.txt compared only the first 100 files of #1869 and #2055, and only the newest 100 of 103 open PRs; overlap there is unchecked. Checked: the PR diff at ab239fd against the previously reviewed d6ff195..e0b7c0b (identical modulo hunk offsets; the two new commits are merges of the base branch); compose.py after the auto-merge (both funnel lines read LGTM |
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
PR #2045 -- fix(report): count dispatches, kernel outcomes and critic passes once each
What it does: the session report counted one logical event under two keys in four places -- a specialist dispatch recorded both as a phase action row and as a framework run, a kernel attempt added both to its outcome bucket and to reverts, e2e_gain taken over reverted attempts, and a funnel stage called optimized that counted every kernel any lane tried. Each counter is now keyed on the single identity or settled outcome that defines it (dispatches by task_id/run_id, attempts by outcome, gain over adopted attempts only, optimized renamed attempted), and the critic table is ordered by ts and numbered 1..N because iter restarts on resume. All of it sits in the reporter layer plus the critic row ordering in the assembler; no persisted field changes.
Blocking issues: 1
- [C2] An attempt settled
adoptedwhose end-to-end decision isREVERTis counted as a keep, and its REVERT is dropped [verified]
Problem:source_counterstests the outcome before the decision, so an attempt carryingoutcome="adopted"together withe2e.decision="REVERT"takes the first branch, lands inkeepsand never reaches theelif decision == "REVERT"below it. src/hyperloom/inference_optimizer/breakdown/reporters/_renderers/_kernels.py:113-123
The shape is what the GEAK revalidation path produces:promote_geak_from_candidatelifts the candidate and then, when the overlay was not proven loaded, callsreject_geak_kernel_journey(orchestrator/phases/kernel.py:2275-2285), which rewrites every journey KEEP todecision="REVERT"(breakdown/recorder/kernel_event.py:358-400), while the rebench ledger row for the same visit carriesdecision="validated"(orchestrator/loop/writeback.py:2822-2833)._settleconsults the rebench ledger beforee2e(breakdown/recorder/kernel_event.py:1777-1790), so the attempt staysadopted.
Impact: the capability summary reports a revoked GEAK keep as an adoption. On the same one-attempt document, base a92a7f2 renders| geak | tried | 1 | 0 | reverts=1 |andcapabilities_kept = []; head ab239fd renders| geak | kept | 1 | 1 | |andcapabilities_kept = ['geak'], while the kernel-pipeline funnel on the same page still saysadopted=0, partial=1.reverts-- the counter this PR narrows to REVERT decisions -- shows 0 for a REVERT. The new testtest_each_attempt_lands_in_exactly_one_outcome_bucketcoversadopted+KEEP,adopted+<none>andrejected+REVERT, so CI stays green.
Action: settle the conflict before bucketing -- let aREVERTdecision win over anadoptedoutcome (or route the pair toreverts/micro_only_keepsexplicitly) and add theadopted+REVERTcase to the bucket test.
SKIPPED: V4 -- "#1869 changes 518 files, only its first 100 were compared", overlap there unchecked.
SKIPPED: V4 -- "#2055 changes 149 files, only its first 100 were compared", overlap there unchecked.
SKIPPED: V4 -- "AMD-AGI/Hyperloom has 107 open PRs, only the newest 100 were compared", overlap there unchecked.
Checked: _kernels.py source_counters, capability_summary._kernel_row/_status, _framework.specialist_tally against phase_event.py actions.rows/task_id and phases/framework.py record_run(task_id), critic.py row numbering and last-pass heading, assembler._compose_critic sort key, cross_section._kernel_funnel and compose.py funnel lines, kernel_event._settle / reject_geak_attempts / _merge_acceptances, tree-wide sweep for readers of the old funnel key and of DEFAULT_IDLE_BODY | Ran: source_counters, capability_rows, render_session_report and kernel_event._settle called directly on constructed documents in both trees; a full recorder-level replay in production call order | Base: a92a7f2 | Head: ab239fd
A REVERT end-to-end decision now outranks an adopted outcome, so a keep the integrate path revoked is no longer counted as adopted. The funnel marks such a kernel reverted instead of partial.
|
Addressed [C2]. Reproduced it: a single attempt with A |
Addressed at fc6c44a: the revoked keep is now counted as a revert.
PR #2045 -- fix(report): count dispatches, kernel outcomes and critic passes once each (re-review at fc6c44a)What it does: the session report counted one logical event under two keys in four places -- a specialist dispatch recorded both as a phase action row and as a framework run, a kernel attempt added both to its outcome bucket and to Blocking issues: none. The finding from the previous round is fixed at fc6c44a. The capability row and the funnel now agree, and Checked: LGTM |
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
No blocking issues at fc6c44a. The revoked-keep finding from the previous round is fixed and verified: an attempt with outcome "adopted" and e2e.decision "REVERT" now lands in reverts, the capability row reads "| geak | tried | 1 | 0 | reverts=1 |" with capabilities_kept empty, and the funnel reports reverted=1, partial=0 on the same page. Details and the full checked/ran list are in the preceding comment.
With prior.txt and the delta in hand, a re-review now settles the earlier one before its own (references/re-review.md): - It decides every finding of the latest card at this head into prior_verdicts.txt, FIXED, HOLDS or MOVED. A finding that holds goes back on the card under its earlier key, opening `held since round <n>.` - It still reviews the whole diff, as a first review does, and reads the delta first: an earlier card can have missed anything, the PR author's own included. On #2045 the author's clean card and the head on which a second reviewer then found a blocking defect carry the same patch. - A new finding on a line the earlier card already had says `(missed at <sha>)`, so a miss is named rather than presented as new, and two cards on one head no longer contradict each other in silence. - From round 3, a finding of a class an earlier round raised names the shared cause in its Action and asks the author to reconsider the approach. The card contract: a finding names one shape and lists every path:line it has in the diff; the Checked line adds Round and, from round 2, Prior (`Prior: C1 fixed, T2 holds`), so the author reads what was fixed without inferring it from a missing line. pr-review-publish.sh refuses a card for a PR with earlier cards when prior_verdicts.txt is empty or Prior is missing, and the CI contract asks for both. SKILL.md stays within its 350-line budget: the CI failure classification, read only when ci.txt or mergeable is quoted, moves to references/ci.md unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
.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>
The session report counted several things more than once or under the wrong name. All fixes are in the reporter layer plus the critic row ordering in the assembler; no schema change.
task_id/run_idset.reverts(any non-KEEP e2e decision) and again inrejected(its outcome); glm52 printedreverts=1 · rejected=3for 0 and 3. Each attempt now lands in one bucket from its settledoutcome;revertscounts onlyREVERTdecisions, and a REVERT decision outranks an adopted outcome, so a revoked keep is counted as a revert (not a keep) and the funnel marks its kernelrevertedrather thanpartial. An attempt whose e2e decision was KEEP but whose outcome is rejected is no longer counted as kept (custom:kept 2/2becomestried 2/0).e2e_gaintook the best gain over every attempt, reverted ones included; it now reads adopted attempts only.optimizedcounted kernels any lane tried, including all-rejected ones (glm52optimized=4with 3 rejected). It is renamedattemptedin the funnel, the summary lines andGlobalFacts.kernel_pipeline_funnel.(iter, ts), butiterrestarts with the process on resume, so ds-v4-pro had 801 out-of-order rows and repeated iter values. The assembler now orders passes byts; the report numbers them 1..N under a#column (and the last-pass heading uses the same number); the idle placeholderok (critic)(DEFAULT_IDLE_BODY) renders as—. The recorder still stores the placeholder. The constant is defined once ininference_optimizer/protocol/intent.py(the layer both the critic runtime and the renderer may import) andintent_envelope.pyre-exports it, so the layer contract holds.fix/report-render-field-mismatch), retarget tomainonce fix(report): render fields on the axis and scope they were recorded on #2043 merges.test_breakdown_renderers.py,test_critic_renderer_unit.py,test_breakdown_assembler_compose.py,test_reporters_smoke.py, each failing on the base branch (exact counts, exact header rows);test_sbd_v6_critic_recording.py's ordering test now expects record-time order.test_breakdown_report_integrity.py::test_capability_table_shows_unadopted_outcomesasserted that a reverted candidate's gain is shown; it now pins the exact row without it. Ran those six files individually;ruffandscripts/code_metrics.pypass. Re-rendered the seven audited sessions: specialist rounds 91→88, 104→100, 31→27, 18→13, 61→56, 27→24, 38→32.GlobalFacts.kernel_pipeline_funnel["optimized"](in-process LLM fact pack) is renamedattempted;session_breakdown.jsoncritic.iterationsis ordered bytsinstead ofiter.