Repository navigation
determinism: name the hrx decode-width gate (engine#751), and put the NPU into the log-prob comparison - #831
Conversation
…es it Measurements for engine#751's HRX residue. GGML_HRX_LOG_DISPATCH=1, keyed on the root node's shape so decode width can be separated from prefill width, over one hrx serve at --parallel 4: the decode step uses two disjoint registration sets at width 1 and width >= 2, and the difference is where the fusions sit (residual add + RMSNorm is one kernel at width 1, two dispatches at width 2; gate/up/SwiGLU and the K-quant matmul likewise switch families). Attention is the same registration at both widths, so unlike cpu it is not the cause here. Six GGML_HRX_DISABLE_DISPATCH arms, each verified to have taken effect by the changed registration set, all still leave the width-2 numbers displaced from solo (max |dlogp| 0.045-0.132 nats). Forcing both widths onto one kernel set is not expressible: hip.dense_kquant_swiglu has no fallback and GGML_HRX_STRICT=1 refuses the graph. --parallel 1 remains the reproducible configuration.
|
Review request for the PR queue: docs-only change ( |
PR Reviewer Guide 🔍(Review updated until commit 84640b5)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 783eec67d — one item: six of the nine registration names in the table do not exist at the pins
this engine ships, so the table cannot be checked against the thing it describes.
What I verified first (so the objection is narrow): GGML_HRX_LOG_DISPATCH is real and where the doc says —
ggml/src/ggml-hrx/dispatch_registration/dispatch-registry.cpp:91 (std::getenv("GGML_HRX_LOG_DISPATCH")), printing
ggml_hrx dispatch: %s (%s) at :100; GGML_HRX_DISABLE_DISPATCH is read in the same file; GGML_HRX_STRICT is in
ggml-hrx.cpp. Four of the nine names are literal registration strings at the shipped pin
(common.rmsnorm_binary_f32, kquant.mul_mat.decode_f32, common.mul_mat.f32_f32_decode,
common.flash_attention_decode_split_next_q8).
The other six are not. At third_party/llama.cpp = d8bd0ff6052a (the pin engine main carries today,
git ls-tree origin/main third_party/llama.cpp) there are 541 distinct "<family>.<name>" registration literals under
ggml/src/ggml-hrx, and only two of them start with hip. (hip.scale_f32, hip.top_k_f32). gemv_add_norm,
kquant_tokens, dense_kquant_swiglu and qkv_rope (as a registration) appear nowhere in the tree — not in the
dispatch registrations, not in the loom corpus — so:
hip.gemv_add_norm absent (hip.qkv_rope, hip.dense_kquant_swiglu,
hip.qkv_rope absent hip.kquant_tokens.mm, .mm_add, .swiglu likewise)
hip.dense_kquant_swiglu absent
hip.kquant_tokens.mm absent
hip.kquant_tokens.mm_add absent
hip.kquant_tokens.swiglu absent
Same result at the newer fork commit the closed #759 bump carried (37ca4d442ad4) and in hrx-system =
82804197bcbe (grep sanity there: 1,291 files match dispatch, so the search is not silently empty). So either the
ablation was run on a build whose fork differs from every pin in this repo, or those six are labels from the lane's
reading rather than strings a reader will see. Either way a docs PR whose whole value is "the names you will see in the
log" has to name strings that GGML_HRX_LOG_DISPATCH=1 actually prints at a stated pin.
Fix: the names as printed at llama.cpp d8bd0ff6052a / hrx-system 82804197bcbe (four are already right), or one
line saying which fork commit the table was measured on. Also dispatch_registry/dispatch-registry.cpp → the real path
is ggml/src/ggml-hrx/dispatch_registration/dispatch-registry.cpp.
Not verified by me, and labelled as such rather than objected to: the logp displacement numbers (0.045-0.132 nats, 0.092
unmodified, 43-48 of 48 bits) — those need an hrx serve on this box's GPU plus the throwaway width-keyed log patch,
which is the author's run, and I took no GPU claim this pass. The rest of the doc is consistent with
docs/determinism.md's existing text and does not promise a switch that does not exist, which is the right shape.
pr_agent failed on this head (20 s); build 6m0s pass, CodeQL pass, all four Analyze pass.
…cross routes by token text
Two flags, both needed to compare the NPU with the other two routes at all:
--thinking on sends enable_thinking=true. The NPU forward path ignores the kwarg, so its
stream always opens <think>; the llama-server routes honour it, so they must be
asked for the same template behaviour or the tokens diverge at position 0.
--match-by text compares tokens by detokenised text instead of id. The NPU reports no token ids,
so an id-keyed comparison between it and cpu/hrx matches on nothing at all.
Defaults are unchanged (thinking off, match by id), so existing reports still reproduce.
--selftest covers both: ids differing while text agrees is a match under --match-by text, a
divergence under the default.
|
Persistent review updated to latest commit f8a5699 |
…kens matched by text The page said the NPU route could not be compared to cpu/hrx, for four reasons. Two of those were properties of how the comparison was run, not of the NPU: run every route with thinking on (--thinking on) and the templates agree, and match by token text (--match-by text) because the NPU reports no ids. content being empty does not matter — the log-probs are in logprobs.content. Measured, Qwen3-0.6B, 8 prompts x 24 tokens, greedy, thinking on, 2 repeats NPU / 5 cpu,hrx: pair token match exact-bit max|dlogp| mean|dlogp| prompts diverging cpu vs hrx 0.958 0.060 6.25e-01 2.83e-02 1 of 8 cpu vs npu 0.641 0.000 6.71e-01 4.66e-02 7 of 8 hrx vs npu 0.641 0.000 6.55e-01 4.21e-02 7 of 8 npu is bitwise repeatable at 24 tokens too (192 tokens, max 0). Control: cpu measured in two separate server runs agreed 192/192 exact bits, so comparing across runs (the GPU and NPU claims are held separately) does not manufacture a difference. The NPU number is 1.6x the cpu-hrx distance and shares no exact bits with either, and the pack is q4_1 requantised from Q4_K: it is 'NPU pack vs GGUF', not an arithmetic-only distance. Stated as such, with the build caveat (44d849d onward masks think tokens, so #832 has to be fixed first).
|
Review requested: this is the lane's determinism work (hrx width gate by name + the NPU row in the log-prob comparison). Docs + two flags on tools/logprob_agree.py, |
|
Persistent review updated to latest commit f2f34c5 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f2f34c5b — my CHANGES on the old head is closed: every registration name the page now prints exists
in the shipped libggml-hrx.so, and the two tool flags it tells you to use exist and their logic self-tests.
What closed the earlier item: at 783eec67d the table named nine registrations and six of them appeared nowhere at the
pins (I had grepped the fork and hrx-system). The names come from the private gpu-kernels addon, which is why a
fork-only grep misses them — and they are all real:
$ strings build/bin/libggml-hrx.so.0.18.0 | grep -c -x -F '<name>'
hip.qkv_rope 1 kquant.mul_mat.decode_f32 1 hip.dense_kquant_swiglu 1 hip.gemv_add_norm 1
common.mul_mat.f32_f32_decode 1 hip.kquant_tokens.mm 1 hip.kquant_tokens.mm_add 1
hip.kquant_tokens.swiglu 1 common.rmsnorm_binary_f32 1 common.flash_attention_decode_split_next_q8 1
ten of ten present, one occurrence each, in the binary that the addon builds. The matcher priorities the text leans on
are also there: hip.gemv_add_norm above kquant.mul_mat_add.decode_f32/common.mul_mat_postops, and
hip.dense_kquant_swiglu at 330, above the Loom gate/up matchers at 290..315 and the kquant swiglu at 315
(addons/hip-hrx/dispatch-hip-dense-swiglu.cpp:245), which is what makes "no fallback" a property of the registration
set rather than a guess.
Checked on the tool side: python3 tools/logprob_agree.py --selftest -> selftest: ok, rc 0; --thinking {on,off} and
--match-by {id,text} are in --help with the descriptions the page gives. The two engine facts the page attributes to
other work also hold: 44d849d is "npu: logprobs entries only for content tokens … (#748)", and issue #832
("forward-serve accepts chat_template_kwargs.enable_thinking and silently ignores it …") is open with exactly the
consequence the page draws from it.
Not re-derived by me, and stated here so the page's provenance is clear: the agreement tables (cpu vs hrx,
cpu vs npu, hrx vs npu), the 26-registration width split, the six GGML_HRX_DISABLE_DISPATCH= arms and the
0.045-0.132 nats they move. Those need one hrx serve and one NPU serve held at the same time, which is a GPU claim plus
the NPU device lock; I took neither, and the lane's own run is the record for those numbers.
Two things I would keep an eye on rather than gate on: the page now says the NPU "can join the agreement comparison",
and it does — but the pair numbers (0.641 token match, 4.2-4.7e-02 mean |Δlogp|) are a pack-vs-GGUF distance, and the
page says so three times in different words; anyone quoting 0.641 as an NPU accuracy number will still be quoting it
wrongly. And --repeat 2 on the NPU side is a small sample for "bitwise repeatable"; it is 192 tokens and it is honest
about the 0.5 s/token cost, but no NPU route has had the 5-repeat treatment cpu and hrx got.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8c5c787 (the main merge; own files unchanged).
Re-verified rather than assumed: docs/determinism.md and tools/logprob_agree.py at this head are byte-identical to
the blobs I verdicted at f2f34c5 (git rev-parse f2f34c5:… == HEAD:… for both), so the merge brought in main and changed
nothing of this PR's. Re-ran the cheap gate here anyway: python3 tools/logprob_agree.py --selftest -> selftest: ok,
tools/copyright.py --check rc 0.
The substance of my 19:19Z READY stands unchanged: all ten registration names the page prints are in the shipped
libggml-hrx.so (hip.qkv_rope, kquant.mul_mat.decode_f32, hip.dense_kquant_swiglu, hip.gemv_add_norm,
common.mul_mat.f32_f32_decode, hip.kquant_tokens.{mm,mm_add,swiglu}, common.rmsnorm_binary_f32,
common.flash_attention_decode_split_next_q8 — one occurrence each), --thinking/--match-by exist, 44d849d and issue
#832 check out. The agreement tables still need a GPU serve plus the NPU serve, which is the lane's run; I took neither then
and take neither now.
|
Persistent review updated to latest commit 8c5c787 |
|
Persistent review updated to latest commit 0af4632 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0af4632ce (the main merge; author diff byte-identical to the head I verdicted: 2 files, docs/determinism.md +127/−32, tools/logprob_agree.py +44/−23 — compared from the merge base at both heads, so the move added nothing of its own).
Re-run at this head, not carried:
python3 tools/logprob_agree.py --selftest->logprob_agree selftest: ok, rc 0;py_compileclean.- The table's substance, checked against the binary rather than the source: all ten registration names exist exactly once as NUL-terminated strings in
libggml-hrx.sobuilt at the shipped pin (llama-server --version->1 (d8bd0ff60); main'sthird_party/llama.cppgitlink isd8bd0ff6052a):hip.qkv_rope,kquant.mul_mat.decode_f32,hip.dense_kquant_swiglu,hip.gemv_add_norm,common.mul_mat.f32_f32_decode,hip.kquant_tokens.{mm,mm_add,swiglu},common.rmsnorm_binary_f32,common.flash_attention_decode_split_next_q8. - CI at this head:
build,CodeQL,Analyzex4 andpr_agentall success.
Same two notes as before, neither blocking: the agreement tables themselves need a GPU plus an NPU serve (the lane's run), and the counters it prints are the dispatch gate's, not a throughput claim.
|
Persistent review updated to latest commit efed965 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at efed965a (the main merge after #836; both of this PR's files are blob-identical to the head I verdicted — tools/logprob_agree.py = 5b69eeb5, docs/determinism.md = 4f82bee5 at 0af4632ce and here — so the move added nothing of its own).
Same evidence as the verdict above, which therefore applies to this tree verbatim: logprob_agree.py --selftest -> logprob_agree selftest: ok, rc 0; all ten registration names in the table present exactly once as NUL-terminated strings in libggml-hrx.so built at the shipped pin d8bd0ff6052a; CI green (build, CodeQL, Analyze x4, pr_agent).
|
Persistent review updated to latest commit eecf6a3 |
…utes The thinking-on table measured the NPU at 24 tokens because of its ~0.5 s/token cost. It is bitwise repeatable at 64 too (8x64x2, max |dlogp| = 0), it just costs 586 s in one NPU claim, so the column is 2 repeats at every budget. At one budget the three pairs read: cpu|hrx 0.766 token match / 4.62e-02 mean, cpu|npu 0.264 / 5.51e-02, hrx|npu 0.266 / 5.19e-02; no pair shares an exact bit with the NPU, cpu|hrx shares 3.1%. The NPU's relative distance narrows from 1.6x at 24 tokens to 1.2x at 64 (the cpu|hrx delta rises with stream length too), so the 1.6x figure is now stated per budget instead of once. The cross-run control holds at both budgets: cpu measured in the two separate server runs agrees 512/512 positions, exact bits, max |dlogp| = 0.
… 1bit/determinism-hrx-width
|
Update: Measured today, build | pair | token match | top-1 | exact-bit | max |Δlogp| | mean |Δlogp| | prompts diverging | Three things this changes in the page:
Still not bitwise anywhere and still outside RL-Kernel's |
|
Persistent review updated to latest commit 84640b5 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 84640b533 (re-verdict; the doc grew, the mechanism is now demonstrated rather than asserted)
I re-derived the control the page rests on, instead of reading it. Two separate 1bit serve runs on this box
(CPU route, Qwen3-0.6B-Q4_K_M, --thinking on --match-by text, 8 prompts, 2 repeats per run), compared with the
tool's own comparator over all eight prompts:
24 tokens: positions 192 common 192 token-match 1.000 exact-bit 1.000 max |dlogp| 0.000e+00 text stable True
64 tokens: positions 512 common 512 token-match 1.000 exact-bit 1.000 max |dlogp| 0.000e+00 text stable True
so "the cpu route measured in two separate runs agrees exactly at both budgets" holds, and --thinking on /
--match-by text work as the page says (both runs exit 0, each route individually bitwise repeatable).
Everything else in the new text that can be checked without the array:
- the tables' arithmetic re-derives from their own denominators: 392/512 = 0.766, 135/512 = 0.264, 136/512 = 0.266;
184/192 = 0.958, 123/192 = 0.641; the NPU/cpu-vs-hrxdistance ratios 4.66/2.83 = 1.65 and 5.51/4.62 = 1.19
("1.6x at 24, 1.2x at 64"); the smallest max |dlogp| in any pair is 0.625, so "no pair inside 0.06" is right; - all ten dispatch registration names in the new width-gate table, and both
GGML_HRX_LOG_DISPATCH/
GGML_HRX_DISABLE_DISPATCH, are present as strings in a builtlibggml-hrx.so(5, 3, 5, 5, 1, 2, 1, 1, 1, 1
hits), and 5 + 4 + 17 = the 26 registrations the page counts; - issue #832 is open and is the
enable_thinkingone the page cites, and44d849d("logprobs entries only for
content tokens") is 11 commits after1b3def2, so "measure with a build before it" is coherent.
Two nits, neither a gate:
- the cross-run snippet on the page compares
npu[0]["runs"][0]againsthrx[0]["runs"][0], i.e. prompt 0
only, while both tables are aggregates over 8 prompts: a reader who runs the snippet gets a per-prompt number
and will not match the table. Loop overzip(npu, hrx)(that is what the tool does internally) or say it is
one prompt; - the "mean |dlogp|" columns cannot be re-derived from the rates on the same rows, and no run directory or report
is cited; one line naming where the two--jsonreports are would make the tables auditable the way the rest of
this page is.
Three things, all in this lane's determinism work. Owner merges; nothing here is merged by me.
1.
docs/determinism.md: thehrxdecode-width gate, by name (engine#751 residue)#751 was closed as documented-not-fixed with the HRX cause unlocated. It is located now, by name
rather than by reading code: at decode width 1 and width >= 2 HRX dispatches two disjoint sets of
fused kernels (
hip.gemv_add_norm+kquant.mul_mat.decode_f32+hip.dense_kquant_swigluversus
hip.kquant_tokens.{mm,mm_add,swiglu}+common.rmsnorm_binary_f32), and no singleGGML_HRX_DISABLE_DISPATCH=switch removes the difference — six arms, each verified to have takeneffect, still leave width-2 displaced by 0.045-0.132 nats. Attention is not the cause on
hrx(the same registration at both widths), unlike on
cpu.--parallel 1is the reproducibleconfiguration. Evidence: engine#751 comment 6582254744.
2.
tools/logprob_agree.py: two flags, so a route that cannot stop thinking can be measured--thinking on— sendsenable_thinking=true. The NPU forward path ignores the kwarg, so itsstream always opens
<think>; the llama-server routes honour it. Without this the two diverge atposition 0 for template reasons, which reads exactly like a numerical disagreement.
--match-by text— compares tokens by detokenised text. The NPU reports no token ids at all, soan id-keyed comparison between it and
cpu/hrxmatches nothing.Defaults unchanged (thinking off, match by id), so every existing report reproduces.
--selftestcovers both new paths;
tools/copyright.py --checkrc 0.3. The NPU joins the comparison — measured
The page claimed the NPU could not be compared. Two of its four reasons were how the comparison was
run, not the NPU; measured now (Qwen3-0.6B, 8 prompts x 24 tokens, greedy, thinking on, 2 repeats
NPU / 5 cpu,hrx):
cpuvshrxcpuvsnpuhrxvsnpuThe NPU is bitwise repeatable at this budget too (192 tokens, max abs dlogp = 0). Control:
cpumeasured in two separate server runs agreed 192/192 on exact bits, so comparing across runs (the GPU
and NPU claims are held separately) does not manufacture a difference.
The NPU distance is 1.6x the
cpu-hrxdistance and shares no exact bits with either, and the packis q4_1 requantised from Q4_K — so that row is "NPU pack vs GGUF", not an arithmetic-only distance
like
cpuvshrx. Stated as such in the doc, with the build caveat:44d849donward masks thinktokens, so the row needs a build before it or engine#832 fixed.