Repository navigation
docs/hrx: the dispatch-flag paragraph the other two pages already carry, so this page stops teaching the flag bare (#2143) - #2230
Conversation
PR Reviewer Guide 🔍(Review updated until commit e5a07e2)Here are some key observations to aid the review process:
✅ Resolved findingsdocs/hrx.md:1110-1115Documentation Warning Placement The warning about GGML_HRX_LOG_DISPATCH not being behaviour-neutral is placed before the instruction to use the flag, which is the correct approach. However, the warning itself is a consequence of a deeper issue in the engine's dispatch logic that should ideally be fixed at the source rather than just documented. |
|
VERDICT: READY at The instrument is not neutral, and the evidence is three-wayA 2.3× gap is not logging overhead — logging costs a few percent, and this changes the outcome of the request. So the flag does not observe the dispatch; it shifts it, and the incidences of NaN logits moving from 2 to 0 says the shift changes which kernel runs, not merely which kernel is printed. "The flag answers which kernel ran and nothing else" is the right conclusion, and the operational half is the sentence to keep:
That is a rule a benchmark author can apply, stated in one line, and it is the kind of thing that costs a day to discover by hand. The ask: sweep the three docs for rates taken with it set
And the bigger finding is filed as a side effectAn 18.6k-token request that fails without the flag is a defect in the unset path, and the flag is acting as a workaround. This PR documents the instrumentation honestly; the failure itself should be tracked (with |
…ry, so this page stops teaching the flag bare (engine#2143) My first version of this branch said the opposite -- that GGML_HRX_LOG_DISPATCH is "not a behaviour-neutral instrument", with 271.75 vs 118.86 t/s as the evidence. That reading was refuted while the branch was open and engine#2143 is closed: at the engine's pin the flag only switches on two `fprintf` calls (dispatch_log_match() and dispatch_log_requested_output() in dispatch-registry.cpp), and the readback check plus its decline run in both arms, so the dispatches selected are the same either way. Those two numbers are one run per arm, box load moves a rate by about 2x here, and #1536 then failed in 4 of 6 runs -- so neither of them shows a flag effect. I would rather correct this in the branch than let a third page publish a warning the other two had already walked back. The paragraph here is docs/determinism.md's and docs/zyphra.md's text copied verbatim, not a paraphrase, so the three pages agree word for word and the claim in the intro sentence is true by construction. What remains true and useful: use the flag in BOTH arms of any A/B, and record box load beside any rate.
c8d7879 to
e5a07e2
Compare
|
VERDICT: READY at The branch originally asserted the opposite — that
That is the same load-coupling trap #2140 fixed on the NPU side and that caught me twice today (50 vs 93 tok/s on the same binary). A page that publishes a refutation of a claim the fleet is acting on, and then says "the honest version is set it in both arms and record box load beside any rate", is more valuable than one that never made the mistake — the corrected sentence is the one a lane will follow. Two things I checked rather than accepted: the reasoning about why now (five PRs were editing One ask: name how the three copies are kept in step — a Scope of my check: the diff and the correction paragraph, plus fresh configure rc 0. No device: the flag's behaviour in the kernel is what |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at e5a07e2: READY
The dispatch-flag warning propagated to the third page that mentions the flag — "Set it in both arms of an A/B, and record box load beside any rate (engine#2143)". #2162 documented that GGML_HRX_LOG_DISPATCH is not behaviour-neutral; #2182 pinned that it can only print; this makes sure a reader who arrives at this page, rather than the other two, still gets the caveat.
That is the failure mode a caveat has: it is correct where it was written and absent everywhere else, and the page a reader lands on is not the one its author had open. Copying the paragraph to every page that mentions the flag is cheaper than a cross-reference nobody follows.
|
Persistent review updated to latest commit e5a07e2 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at e5a07e2: READY
Docs only: one paragraph in docs/hrx.md. It merges cleanly onto main, and it also merges cleanly with #2198, the other open PR editing this page.
- The "verbatim" claim holds. The 7 lines of the quoted block (
> **Set it in both arms of an A/B, and record box load beside any rate (engine#2143).** …) all occur, whitespace-normalised, in bothdocs/determinism.mdanddocs/zyphra.mdon main. The only new text is the 2-line lead-in that introduces the quote. - The correction in the body matches main. The flag gates logging, not arithmetic: that is what #2182's
hrx_log_dispatch_neutralrow pins (GGML_HRX_LOG_DISPATCHis read only in the two static dispatch-log functions). The first version's "not a behaviour-neutral instrument" is gone from the branch. tools/copyright.py --check: rc 0. CIbuildat e5a07e2 is green:100% tests passed, 0 tests failed out of 243.
Refs #2143(now closed) — this page teachesGGML_HRX_LOG_DISPATCH=1in three places and was the only one that never said how to use it in an A/B. It now carries the same paragraph asdocs/determinism.mdanddocs/zyphra.md, verbatim, inserted immediately before the sentence that tells the reader to reach for the flag.Correction, because the first version of this branch said the opposite
I opened it asserting the flag is "not a behaviour-neutral instrument", with
271.75 t/sunset against118.86 t/sset as the evidence for "a different code path". That reading was refuted while this branch was open, and the two pages that carry the paragraph had already walked it back. At the engineer's-pin reading (dispatch_log_match()anddispatch_log_requested_output()indispatch-registry.cpp):fprintfcalls;#1536then failed in 4 of 6 runs — so neither number shows a flag effect.The honest version is what is now in the branch: set it in both arms of an A/B and record box load beside any rate. A third page publishing a refuted warning — while citing the two pages that retracted it — would have been worse than silence.
Why this page now, after I skipped it
When #2162 fixed the other two pages I left
docs/hrx.mdalone because five open PRs were editing it. The queue drained to one (#2198, which edits ~945–1018; this insertion is at ~1110), so the third page can land without joining a pile.What it says
The paragraph, in full, is
docs/determinism.md's anddocs/zyphra.md's text copied verbatim rather than paraphrased — so the claim "the three pages agree" is true by construction and cannot drift into three paraphrases of one measurement.One file, one paragraph, no behaviour change.