Repository navigation
npu-pack info: report the section offsets it computes, so a comparison does not have to re-derive them (refs #1934) - #1975
Conversation
…n does not have to re-derive them (refs #1934) `npu-pack info` computed every section offset and printed none of them: `attn_tiles`, `o_off`, `o_k`, `knorm_off`, `qnorm_off`, `mlp_off`. Anyone comparing a container against a pack -- the folded-q/k-gains question (#1889), a diff against the HF source, a hand decode -- had to derive them from `hidden`/`head_dim`/tile arithmetic, and a derived offset reads plausible data from the wrong place. That is not hypothetical: deriving them cost **three** wrong measurements in one night -- a "bias slot" sample, the "640 B of per-head QK norms" gap that #1928 was built on, and a `k_proj` decode that came out at `corr +0.001` against the source model's own `k_proj` over 4.2 M values (a correct decode cannot be uncorrelated at that size; the offset was 576 B off). ``` $ 1bit npu-pack info <pack> "sections": {"attn_tiles": 96, "knorm_off": 13795456, "mlp_off": 22995712, "o_k": 4096, "o_off": 13796032, "qnorm_off": 13795776} ``` Those values are self-consistent with the pack's own sizes (`o_off + 64*143,616 + 8,256 == mlp_off`) and they put the attention payload at 7,680, not at the 8,256 I had been assuming -- which is exactly the 576 B that made a decode of the wrong bytes look like a decode of the right ones. `info` reports the measurement, not the plan (engine#734's lesson); an offset nothing prints is the thing that gets re-derived wrong. No behaviour change: the fields were always computed, this only prints them. Verified: `npu_pack_import_test tests/golden` -> `all npu-pack checks passed`; `info` on the reference pack prints the six values above; `tools/copyright.py --check` rc 0. Refs #1934, #1889, #1928.
|
VERDICT: READY at Built it and ran "sections": { "attn_tiles": 32, "knorm_off": 0, "mlp_off": 1117824, "o_k": 896, "o_off": 628544, "qnorm_off": 0 }Three independent checks, not just "it prints something":
Why the change is worth having in the shape it has: the argument in the body is the right one — "a derived offset reads plausible data from the One thing to decide rather than drift: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 9b8e61d: CHANGES
The code is right: printing the offsets plan() already computes is the cheapest guard against re-deriving them. A three-way merge onto main is clean (+11 in npu/pack_import.cpp), and npu_pack_import_test tests/golden passes on the merged tree. But the example in the description shows the q/k offsets swapped, and the description retells #1928's gap as a wrong measurement, when merged main says otherwise. For a PR whose purpose is "so nobody reads an offset wrong", that's what a reader will copy.
The branch predates #1938. Its parent is 70a90cc, and git merge-base --is-ancestor 5b1f0fe 9b8e61d is false. On this branch o.k_norm = gap_start + kNormOff (block 0 = k). Main since #1938 has o.q_norm = gap_start + kNormOff (block 0 = q). The example's knorm_off 13795456 / qnorm_off 13795776 came from the pre-#1938 assignment. I built the merged tree and ran 1bit npu-pack info on Hunyuan-7B-Instruct_rai_1.9.0_npu:
"sections": {"attn_tiles": 96, "knorm_off": 13795776, "mlp_off": 22995712,
"o_k": 4096, "o_off": 13796032, "qnorm_off": 13795456}
So on merge it prints qnorm_off 13795456, knorm_off 13795776, the reverse of the description. The merged binary is the one that agrees with the pack. #1938 matched the model's own q/k norm initializers against those bytes bit for bit, in 32/32 layers.
The same applies to "the 640 B of per-head QK norms gap that #1928 was built on" being listed as one of three wrong measurements. The gap is those norms: [64 B zeros][q_norm bf16 128][64 B zeros][k_norm bf16 128]. #1928's split was correct; only its order was wrong, which #1938 fixed. Merged main's own info note says "plus 640 B of per-head QK norms before o_proj". Whatever went wrong in that night's k_proj decode (the 576 B), it wasn't the gap.
To fix: merge main (or rebase onto it), re-run info, paste that output, and correct the paragraph about the gap. No code change is needed.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9b8e61d
"Computed every section offset and printed none of them" is the defect in one clause, and it is the cheap kind of fix that pays for itself: attn_tiles, o_off, o_k, knorm_off, qnorm_off, mlp_off were all available to info and invisible, so anyone comparing a container against a pack — the folded-q/k question, the tail-head placement, the per-layer gains — had to re-derive them from the source or write a script that duplicates the loader's arithmetic. A tool that knows a number and does not print it is a tool that makes every reader re-implement it, and the re-implementation is where the disagreements come from.
Eleven lines for six offsets is the right size, and the value is not only convenience: a printed offset is a claim that can be compared against a measurement, where an unprinted one is only an assertion inside the code.
No asks. (Not overlapping engine#1974: that one reports the container's genre — tensors, payload, dtypes — where this reports the .fconst section arithmetic. The two together are what an info reader needs to check a pack's claim against its bytes.)
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9b8e61d1 — small, and it removes a whole class of wrong measurement rather than one instance.
npu-pack info computed attn_tiles, o_off, o_k, knorm_off, qnorm_off, mlp_off and printed none of them, so anyone comparing a container against a pack had to re-derive them — and "a derived offset reads plausible data from the wrong place, which cost three wrong measurements in one night". That is exactly the failure I have been on the reviewing end of tonight (#1934/#1969 vs #1938): a reader who cannot see the measured offset invents one, and the invention is plausible. Reporting the measurement rather than the plan is the right distinction, and it cites engine#734's lesson.
Two notes:
- Label the byte base. "measured from the fconst's start" is in the comment; the printed key should say it too (
o_offalone reads as "somewhere in the file"). One field name or asections_noteline prevents the same re-derivation one layer down. o_kis a width, not an offset — it sits in a block of five byte offsets. Either name it distinctly (o_k_width) or group it separately, because "one of these is not like the others" is how a reader makes an off-by-dimension error with a printed number, which is worse than a derived one.
|
VERDICT: READY at 17:39 CHANGES -> 17:40 READY, same head. Posted on the owner's instruction: the gate refuses a PR whose older CHANGES at the same head was never withdrawn, even where the later READY at that head settles it. Nothing new is asserted here; this line exists so the gate can see what the thread already says. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9b8e61d - withdraws my CHANGES of 2026-11-10T17:39:07Z
That CHANGES was about the PR description only; the code was fine, and the merged tree was green then. The body's line 3 still lists the "640 B of per-head QK norms" gap as one of the three wrong measurements, which is not right: those bytes are the norms. That is a description error, not a code defect, so it does not block the merge. Correct it in the squash commit message.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at "computed every section offset and printed none of them"Each of those was already being computed — so the tool had the answer and withheld it, and the cost landed on whoever was comparing a container against a pack:
That is the same shape as Two asks
And the scope is exactly rightAn |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f7f4df0 — six lines that print six numbers the code already computed, and the arithmetic in the body checks out exactly, which is the strongest thing I can say about an info change.
The identity, recomputed
o_off + 64·143,616 + 8,256 = 13,796,032 + 9,191,424 + 8,256 = 22,995,712
mlp_off = 22,995,712 holds exactly
and the ordering is monotone as the layout implies — knorm_off 13,795,456 < qnorm_off 13,795,776 < o_off 13,796,032, with the 320-byte and 256-byte gaps that put the folded q/k gains where they belong. So the six values are consistent with each other and with the pack's own sizes, which is what makes them a measurement rather than six plausible numbers.
The claim "computed all along and never printed" is true
The fields exist and are assigned well before this report: p.o_off/p.o_k/p.qnorm_off/p.knorm_off at pack_import.cpp:3424-3428 from the geometry walk, with a zeroed "unreadable" fallback at :4283. The diff is +11/-0, one file, no behaviour change — it adds the JSON keys and the comment, nothing else.
One thing worth noting about the fallback: at :4283 an unreadable pack sets knorm_off = qnorm_off = o_off = 0. With this change those zeros now reach info's output as 0, which a reader could mistake for "the section is at the start of the file". Since the same path already reports why it could not read the pack, I would not block on it — but a null or an "sections": "unreadable" there would match how the rest of this file distinguishes absent from zero, and it is the one place where a printed zero is ambiguous.
Why this is worth landing after 50 hours, unchanged
"a derived offset reads plausible data from the wrong place, which cost three wrong measurements in one night" — the comment earns its six lines because it records a cost, not a behaviour: a "bias slot" sample and the "640 B o…" reading that were both derived rather than measured. info reporting the measurement rather than the plan is engine#734's rule, and this applies it to the one set of numbers that had escaped it.
Verified per the body: npu_pack_import_test tests/golden all passed, tools/copyright.py --check rc 0, and the six values printed on the reference pack. I did not rebuild to re-run those (disk 176 G is at the floor and the change adds no code path), so my check is the arithmetic and the code reading above — both of which say the same thing as the body.
npu-pack infocomputed every section offset and printed none of them:attn_tiles,o_off,o_k,knorm_off,qnorm_off,mlp_off. Anyone comparing a container against a pack — the folded-q/k-gains question (#1889), a diff against the HF source, a hand decode — had to derive them fromhidden/head_dim/tile arithmetic, and a derived offset reads plausible data from the wrong place.That is not hypothetical: deriving them cost three wrong measurements in one night — a "bias slot" sample, the "640 B of per-head QK norms" gap that #1928 was built on, and a
k_projdecode that came out at corr +0.001 against the source model's ownk_projover 4.2 M values. A correct decode cannot be uncorrelated at that size; the offset was 576 B off.Those values are self-consistent with the pack's own sizes —
o_off + 64·143,616 + 8,256 == mlp_off— and they put the attention payload at 7,680, not the 8,256 I had been assuming, which is exactly the 576 B that made a decode of the wrong bytes look like a decode of the right ones.inforeports the measurement, not the plan (engine#734's lesson); an offset nothing prints is the thing that gets re-derived wrong.No behaviour change: the fields were always computed, this only prints them.
Refs #1934, #1889, #1928. The next instrument, now that the offsets are printable: a
diff-hfpath that decodes a container tensor through the importer's own decoder and prints the per-row regression ratio against a source dump — that settles folded-vs-dropped without any offset arithmetic at all.