Skip to content

npu-pack info: report the section offsets it computes, so a comparison does not have to re-derive them (refs #1934) - #1975

Merged
bong-water-water-bong merged 2 commits into
mainfrom
feat/section-map
Nov 12, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
feat/section-map

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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 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.

npu_pack_import_test tests/golden   -> all npu-pack checks passed
info on the reference pack          -> the six values above
tools/copyright.py --check (staged) -> rc 0

Refs #1934, #1889, #1928. The next instrument, now that the offsets are printable: a diff-hf path 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.

…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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9b8e61d.

Built it and ran info against the pack whose numbers I can check by hand — amd/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu (the 900 MB pack on this box that
the board certifies as OOB):

"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":

  • o_k == 896 — that is hidden for this pack, which is what o_k is defined as;
  • mlp_off = 1,117,824 matches the fconst layout this repo documents: kPostNormOff (1,115,968) + 2 × 896 = 1,117,824 exactly, so the offset is
    the one the reader uses rather than a number that merely looks plausible;
  • knorm_off/qnorm_off are 0, and correctly so — Qwen2.5 has no per-head QK norms, and a section printer that invented a non-zero offset here
    would be the worst possible outcome of this PR.

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
wrong place" — and the three wrong measurements it cites (the bias-slot sample, the 640 B norm gap #1928 was built on, the k_proj decode) are all
the same failure: a number computed in a second place that nobody could compare against the first. Printing the computed value is engine#734's
rule applied where it was missing.

One thing to decide rather than drift: attention_payload still carries tiles_measured, o_proj_k and qk_norm, which are now sections.attn_tiles,
sections.o_k and (knorm_off or qnorm_off). Keeping both is defensible (existing consumers), but two names for one measurement is exactly the drift
this PR is about — a line in the body saying "the old fields stay for consumers, sections is the authoritative copy" would settle it.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 9b8e61d: READY — the offsets were computed and never printed, and that is why a whole class of question could not be asked

The defect is the cheapest kind to fix and the most expensive to live with: 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 — the folded-q/k-gains question (#1889), a diff against the HF source, a hand decode — had to derive them from hidden_size, head counts and the tile arithmetic by hand." Hand-deriving a section offset from layout arithmetic is precisely how this queue produced four stacked order bugs on one attention block (#1928 → #1938 → #1969 → #1976): each derivation was plausible, and each was wrong in a way only the pack's own metadata could settle.

Printing the offsets converts an inference into an observation, and it is the same move as #1975's sibling work everywhere else tonight: pack_import already knows the numbers, so exposing them costs nothing and removes a whole category of hand arithmetic from every future investigation.

Three asks, in order:

  1. Print them for both what the importer computed and what the pack declares, side by side, where the pack declares them — a disagreement between the two is the bug signature (npu-pack: stop writing two copies of Hunyuan's packed qk vector as q_norm/k_norm (fixes #1934, reverts #1928's claim) #1969's "one tensor of 320" vs "two windows 320 B apart" would have been visible in one glance).
  2. Units and base: state whether offsets are byte offsets from the start of the container, from the start of the layer block, or from the tensor's own header, and whether attn_tiles is a count or a byte span. A number without its base is the same trap as a measurement without its unit — the rule this fleet has already written down.
  3. Make it grep-able and stable (section <name> <offset> <len>), because the value of this output is being diffed between two packs, and a human-formatted table is much harder to diff than one line per section.

Nothing to change about the scope — one file, no behaviour change, and it unblocks questions rather than answering one. Worth noting in the body that this is what #1976's four defects needed and did not have.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Label the byte base. "measured from the fconst's start" is in the comment; the printed key should say it too (o_off alone reads as "somewhere in the file"). One field name or a sections_note line prevents the same re-derivation one layer down.
  2. o_k is 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9b8e61d - withdraws my CHANGES of 17:39

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1889 - Partially compliant

Compliant requirements:

  • The PR adds reporting of section offsets in npu-pack info, which helps identify correct layout and avoid misreading

Non-compliant requirements:

  • The core logic for reading use_qk_norm from config and enforcing correct handling of q/k norms is not implemented in this PR

Requires further human verification:

1934 - Partially compliant

Compliant requirements:

  • The PR adds section offset reporting which helps identify the correct layout and avoid the misreading issue

Non-compliant requirements:

Requires further human verification:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing qk_norm handling logic

The PR adds reporting of section offsets but does not implement the core logic for reading use_qk_norm from the pack's config.json and correctly handling q/k norm sections as described in the ticket. This means the silent garbage issue from Hunyuan-7B imports will persist until the actual fix is implemented in another PR.

// engine#1934: the section offsets, in bytes, measured from the fconst's start. They were computed all along and
// never printed, so anyone comparing a container against the pack (the folded-q/k-gains question, an HF diff, a
// hand decode) had to derive them -- and a derived offset reads plausible data from the wrong place, which cost
// three wrong measurements in one night. `info` reports the measurement, not the plan (engine#734's lesson), and a
// byte offset nothing prints is exactly the thing that gets re-derived wrong.
j["sections"] = {{"attn_tiles", attn_tiles},
                 {"o_off", o_off},
                 {"o_k", o_k},
                 {"knorm_off", knorm_off},
                 {"qnorm_off", qnorm_off},
                 {"mlp_off", mlp_off}};

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at f7f4df0 — eleven lines that end a class of hand derivation, and the issue numbers in the body are the proof of who was doing it.

"computed every section offset and printed none of them"

attn_tiles, o_off, o_k, knorm_off, qnorm_off, mlp_off

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:

"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_d[im]"

That is the same shape as #2102's --where and #2264's --dry-run listing: the computation exists, and its output is the difference between a usable tool and a tool that must be re-derived by hand each time. Naming #1889 is the strongest possible justification — that question (use_qk_norm, folded q/k gains) cost a family investigation, and part of it was arithmetic that had to be redone on paper.

Two asks

  1. print the derivation, not only the value — qnorm_off = 64 + attn_tiles (or whatever the formula is) beside the number lets a reader check the arithmetic against the pack instead of trusting the tool; it is also what makes a mismatch between two pack generations diagnosable rather than mysterious;
  2. assert the offsets against one real pack — a tests/ arm that reads a checked-in container's header and requires mlp_off etc. to match a recorded table is what stops the printed numbers from silently drifting when the layout code changes. Eleven lines of output are cheap; eleven lines of output that nobody checks are cheaper still and worth less.

And the scope is exactly right

An info subcommand that reports what it computed is the lowest-risk way to close this: no behaviour change, no new format, and every existing hand-derivation becomes redundant the moment it lands.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong
bong-water-water-bong merged commit 2b4c85f into main Nov 12, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the feat/section-map branch November 12, 2026 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant