Repository navigation
fix(report): render fields on the axis and scope they were recorded on - #2043
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.
Code metrics gate: PASSEDJudged: the 12 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: |
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.
PR #2043 -- fix(report): render fields on the axis and scope they were recorded onWhat it does: The session report and LLM fact pack printed several fields on the wrong axis or scope: a tok/s headline beside an interactivity gain, one leg's elapsed time as the session's, the stop-target dict as the objective, a zero TTFT/speedup as a reading, and failed roofline runs as "no data". This PR fixes each one in the reporter layer only. On an interactivity-graded session the headline now reads before → after from the settled row of the stack ledger that produced the gain, used only when it is on the same axis and reproduces the gain; otherwise it prints only the gain and the axis. Blocking issues: none Checked: diff vs merge base and the two follow-up commits vs round-1 head f0a803c; cross_section.py (_settled_pair, _graded_axis_headline, _headline, build_global_facts, GlobalFacts), compose.py, _renderers/{final,baseline,session,workload,roofline,_kernels}.py, base.py (events_of, stop_target_text, measured_ms), llm_prompt.py. Confirmed the settled row is the row that produced the gain: the collector (collectors/v6.py LGTM |
PR #2043 -- fix(report): render fields on the axis and scope they were recorded onWhat it does: The session report and LLM fact pack printed several fields on the wrong axis or scope: a tok/s headline beside an interactivity gain, one leg's elapsed time as the session's, the stop-target dict as the objective, a zero TTFT/speedup as a reading, and failed roofline runs as "no data". This PR fixes each one in the reporter layer only. On an interactivity-graded session the headline now reads before → after from the settled row of the stack ledger that produced the gain, used only when it is on the same axis and reproduces the gain; otherwise it prints only the gain and the axis. Blocking issues: none Checked: the diff against the new base LGTM |
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
What it does
The session report printed several fields on a different axis or scope than the one they were recorded on: the headline paired output tok/s with a gain measured on interactivity, the summary showed one leg's elapsed_minutes as the session time, the workload line printed the raw stop-target dict, a workload that measures no TTFT had its recorded 0 printed as a reading, the Final kv paired raw throughput with the display unit, a session with no validation got both "validation predates the final adoptions" and "no validation run completed", an unmeasured kernel speedup rendered as 0.00x, an all-failed roofline section read as "no data recorded", and the LLM prompt named an attribution_method value that is never emitted. Every fix is in the reporter layer — the recorded fields are correct, only the render-time pairing was not.
Blocking issues: 1
- The branch conflicts with
mainand cannot be merged as it stands.mergeable=CONFLICTING/mergeStateStatus=DIRTYat heada92a7f2, andgit merge-tree origin/main a92a7f2conflicts inscripts/code_metrics_baseline.txt. The conflict is textual and confined to that file: this PR lowers three entries (final.py::render42→37 and 178→175,session.py::render93→92) whilemainhas since deleted neighbouring entries in both hunks (kernel_lifecycle.py::render,collectors/v6.py,recorder/framework_event.py,recorder/instrument.py), so the context around each edit no longer exists onmain. Please mergemainin and re-resolve that file — it is the input to the code-metrics gate, so the resolution decides whether the gate stays green.
No other blocking issue. The nine claimed fixes each do what the description says, and the description matches the diff at head.
Checked
cross_section.py_settled_pair/_graded_axis_headline: the before/after pair comes from the same settled row of the same (last)stackevent thatcollectors/v6.py:483-528readsvalidated_total_gain_pctandgraded_onoff, so the pair and the gain cannot be cross-wired.writeback.py:738-765is the only caller ofrecord_validationand passesgraded.reference/graded.candidate/graded.objectivewithvalidated_gain_pct = (candidate-reference)/reference*100of that exact pair, which makes the_PAIR_TOLERANCEcheck a float round-trip of an identity: it cannot admit a contradicting pair nor reject the true one. Axis coverage is complete —total_throughputis "reported, not graded" (schema.py:147), so the graded set is {interactivity axes,output_throughput}.final.py_validation_gap_warnings: the newvalidated=val_stack_len is not Nonegate cannot silence a real stale-validation warning —recorder/stack_event.py:290writesstack_lenunconditionally on every settled row andto_int(0)returns0, so the gate means "a settled row exists", which is exactly the discrimination the fix needs.base.pymeasured_ms/_kernels.py_measured_speedup: applied at every render site —baseline.py:108/174andfinal.py:259for TTFT, and the singleentry["speedup"]read at_kernels.py:205for the kernel table (kernel_lifecycle._fmt_speedup(None)→—).baseline.py:147still warns on an explicit null, so absent and zero stay distinguishable.- Final throughput unit:
framework_registry.throughput_unit(fw)is the same string the Baseline section prints, becauserecorder/baseline_event.py:226records the measurement unit from that same function; the oldprimary_metric_unitis"ms"on a scriptable framework, which is the mislabel being fixed, and it is still used for the delta line atfinal.py:168. GlobalFacts.objectiveremoval: the only readers werecompose.py:212-218andcompose.py:278-288, both converted;as_prompt_dictisasdict(self), so the LLM payload follows the dataclass. No surviving reader insrc/,docs/or tests.roofline.py_failed_runs_note:actionsis one row per dispatch (roofline_event.py:889-929, emptyfailuredicts normalised toNone), so the count and the "all failed" wording cannot overcount; a session with no roofline event keeps the old wording.- Description against diff at head: the two trailing commits are base merges, the nine numbered claims are all in the diff, the added test count (29) matches, and the three lowered
code_metrics_baselineentries are the ones the body names.
Ran
gh pr view 2043; git merge-tree --write-tree origin/main a92a7f2; git diff 958874f origin/main -- scripts/code_metrics_baseline.txt; tree-wide greps for objective, elapsed_minutes, ttft_mean_ms, speedup, throughput_unit, validated_at_stack_len, GlobalFacts. Local pytest was not run (CI is green at a92a7f2: 12/12 test shards on 3.10 and 3.11, coverage, code-metrics, mypy, ruff, pr-hygiene); test behaviour was checked by reading the producer of each asserted field.
Base: 958874f20e98e5d6a80d0960024f6caf6e5580fb · Head: a92a7f2ac5154b6d4f001daf0fdfcdcde5665f20
|
Addressed the blocking issue: |
Resolved at head c65b158: the base conflict in scripts/code_metrics_baseline.txt is gone and the branch now merges cleanly.
|
Re-reviewed at head Blocking issues: noneThe base conflict is gone: Checked
Ran
Base: LGTM |
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
Approving at head c65b158. No blocking issues.
The base conflict from my earlier review is resolved: mergeable=MERGEABLE, the resolution in scripts/code_metrics_baseline.txt keeps main's deletions and re-applies only this PR's three lowered entries, and the src/ diff is byte-identical to the previously reviewed head. CI is green at this head (12/12 test shards on 3.10 and 3.11, coverage, code-metrics, ruff, pylint, mypy, import-linter, CodeQL, bandit, gitleaks, reuse, pr-hygiene, docs).
Details of what was checked are in my comment above.
The session report (
session_report.mdand the LLM fact pack) printed several fields on a different axis or scope than the one they were recorded on. Each fix is in the reporter layer only; no recorder, collector or schema change.outcome.final.graded_onis an interactivity axis, the headline now reads that axis, with before and after taken from the settled validation row of the laststackevent (validations.settled.baseline_tput -> validated_tput, used only when itsgraded_objectiveis that axis and the pair reproduces the gain within 0.5%):e2e_norm_intvty_p50 63.03 -> 79.76 = +26.55% validated gain. When no consistent pair exists it states only the gain and the axis (+26.55% validated gain on e2e_norm_intvty_p50). A re-anchored baseline therefore no longer yields a before/after that contradicts the gain. Sessions not graded on interactivity keep the tok/s line. The Final section'sValidated cumulative gainnames the axis the same way.elapsed_minutes(1602) as the session time. It now preferstotal_elapsed_minutes(2742) and labels itelapsed(total); the Session section lists both under their own names.{'kind': 'gain_pct', 'value': 500.0}) as the objective. It now printsgraded_on(outcome.final.graded_on,unsetwhen none was recorded) and the stop target asgain_pct>=500(all targets of a multi-target run,baseline=<dir>for a baseline target,time_onlywithout a value).GlobalFacts.objectiveis replaced bygraded_onandstop_target.ttft_mean_ms = 0, which was printed as a reading. A non-positive TTFT is now treated as unmeasured in the Baseline and Final sections (no line, and no null warning).10.25 mson a scriptable run). It now usesframework_registry.throughput_unit, the same unit the Baseline section prints.speedup = 0.0rendered as0.00x ... [KEEP]. A non-positive speedup now renders as—.[roofline] skipped: no data recorded this session.The skipped section now states3 run(s), all failed: trace_analyze_failed×2, profile_failed×1.attribution_methodother than"validated"not validated, a value never emitted. It now namesstack_ledgeras the validated method and lists the real other values.output_throughputrename, schema 6.1), which is merged; the base branch ismain.test_reporters_smoke.py, throughrender_session_report/build_global_facts/SYSTEM_PROMPT; the ones pinning a changed line fail on the base branch. Removals are asserted with exact lines or lists. Rantest_reporters_smoke.py,test_roofline_renderer_unit.py,test_breakdown_renderers.py,test_breakdown_report_integrity.py,test_reporters_llm_client.py,test_phase_timeline_renderer.py,test_critic_renderer_unit.py,test_coverage_margin_unit.py,test_coverage_margin3_unit.py,test_param_search_renderer_units.pyindividually;ruffandscripts/code_metrics.pypass. Re-rendered the seven audited real sessions to check each line before and after.scripts/code_metrics_baseline.txtlowers three entries (cognitive complexity and function length offinal.py::render, function length ofsession.py::render) that the extracted helpers shortened.graded_onandstop_targetinstead ofobjective.GlobalFacts.objective(an in-process fact pack passed to the LLM) is replaced bygraded_onandstop_target.metadata.grading.objective(p90) andgraded_on(p50) disagree in the collector; a duplicatewarm_replayevent id is a recorder issue; old sessions' stack ledger stays mixed-axis (fixed for new sessions by fix(stack): attribute AgentX adoptions on the graded axis #2022).