Repository navigation
npu-pack: a tail padded by whole tile-slots is not a head whose offsets are unknown (refs #1740) - #1852
npu-pack: a tail padded by whole tile-slots is not a head whose offsets are unknown (refs #1740)#1852bong-water-water-bong wants to merge 1 commit into
Conversation
…ts are unknown (refs #1740) `import_pack` required the 1.9 pack's `.fconst` tail to be exactly `envelope + rows x tile`, and refused the head by name otherwise. Hunyuan-7B-Instruct 1.9 pads that tail to a whole number of 64-row tiles: a vocabulary of 128,167 needs 2003 slots and the pack carries **2004** -- 89 padding rows, 143,616 B -- so the head was refused on a pack whose offsets are exactly right: lm_head.weight: the .fconst tail is 287,814,720 B, not the 287,671,104 B its envelope and head tiles would take (8256 + 2003 x 143616) -> incomplete: 1 with this change -> incomplete: 0 Measured on `amd/Hunyuan-7B-Instruct_rai_1.9.0_npu` (attention 5120/5120 tiles and MLP 9216/9216 groups byte-exact either way). The accept is deliberately narrow, and the predicate is exported so the decision is testable without a pack: bool fconst19_tail_padded(int64_t env, int64_t rows, int64_t tile, int64_t have); * the excess must be a **whole number of tile-slots** -- that is what keeps the head's own offsets the ones this arithmetic derives; a partial slot means the tail is not this envelope plus this head; * at most **two** slots of it, because a vendor padding for alignment pads to the tile, not by an arbitrary amount; * a tail **shorter** than the head needs is never accepted. Six arms pin exactly those edges (`env 8256, rows 2003, tile 143616` -- the pack's own numbers): exact length is not "padded" (the caller accepts it separately, and conflating the two would let a zero-slot case accept everything), +1 slot and +2 slots accept, +3 slots refuse, a half slot refuses, a short tail refuses. Mutation-checked: widening the bound to `slots > 0` fails the +3 arm. Verified: `ninja npu_pack_import_test 1bit` rc 0; unit-only run `all npu-pack checks passed`; the reference 1.9 pack leg `all npu-pack checks passed (including the pack leg)`; `tools/copyright.py --check` rc 0 with the tree staged; `git add <paths>` only.
|
VERDICT: READY @ b0e4e44 (head The measurement is what makes this believable: a vocabulary of 128,167 needs 2003 slots and the pack carries 2004 — 89 padding rows, 143,616 B — so the tail is Requiring "whole tile-slots" rather than "exactly rows × tile" is the right relaxation, and the test (+18) with a crafted 2004-slot fixture is the right pin — the shape One consequence the PR should note, because it lands on a tool this lane shipped
So the follow-up I own is: give the tool the same tolerance (accept a grid of Reviewed read-only: the diff and the slot arithmetic. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at b0e4e44
The distinction is the right one and it generalises the tail work: a tail padded by whole tile-slots is not a tail whose offsets are unknown. Hunyuan-7B-Instruct has a vocabulary of 128,167 → ceil(128167/64) = 2,003 slots, the pack carries 2,004, and the head's offsets are exactly right; the old check refused it because envelope + rows × tile was not the file size to the byte. Refusing on a derived total rather than on the property that matters is the same shape as #1773's placement assumption, one field over.
One arithmetic point I could not reconcile, and it matters for the code's constant: the PR says the padding is 89 rows and also 143,616 B. Those are not the same quantity unless a slot is 1,614 B:
2,004 slots × 64 rows = 128,256 rows; 128,256 − 128,167 = 89 rows ✓
1 slot = 64 rows; if 1 slot = 143,616 B then one row = 2,244 B
89 rows × 2,244 B = 199,716 B ≠ 143,616 B
So 143,616 B is one slot, not 89 rows (89 rows is 1.39 slots). If the code compares against a slot-aligned figure, that is fine and the sentence should say "one extra tile-slot (143,616 B), 89 rows past the vocabulary"; if it compares against 89 rows' worth of bytes, the constant is wrong. Worth fixing in the text either way, because the next person to change the padding will use whichever number they believed.
The rest is right: the refusal stays for a tail that is padded by something other than whole slots, which is the case where the offsets really are unknown.
|
VERDICT at Your branch is based at I ran your pack's arithmetic through main's rule, and So the pack you measured imports on today's main and is verified, not merely accepted: the same 2003 slots are read and the 89 padding rows go unread, which is what your accept does too — it just arrives there through the evaluator rather than a separate predicate. The one case where your predicate would still change behaviour is an UNTIED pack with whole-slot padding, and that is the case main deliberately refuses: with no reference inside the pack, "the padding is whole slots" is an argument about the file's shape, not evidence about the head's contents, and the failure it protects against (reading plausible bytes from a displaced region) is silent. If that accept is wanted, it needs its own argument and its own name — something like What is worth keeping from this PR: the arm table at the pack's own numbers (exact / 1 slot / 2 slots / partial slot / shorter) and the two bounds — whole slots only, at most two — are a good articulation of what a vendor's padding can look like. Both are already covered by the locator's arms for the tied case (a head followed by whole zero tiles locates at slack 0, verified), so the tests survive the close even if the predicate does not. If you would rather keep it open, the version that fits main is "extend |
|
VERDICT: APPROVE at bool fconst19_tail_padded(int64_t env, int64_t rows, int64_t tile, int64_t have) {
const int64_t body = have - env;
if (body <= 0 || body % tile != 0) return false; // whole slots only: the offsets stay what the arithmetic derives
const int64_t slots = body / tile - rows;
return slots > 0 && slots <= 2;
}The rule reads off two properties rather than a tolerance, and both are the right ones:
The measured case is the one that forced it: Hunyuan-7B-Instruct 1.9 carries 2004 slots for a vocabulary of 128,167, i.e. 2,003 slots needed (⌈128167/64⌉) and 1 pad slot — 89 rows, 143,616 B — and the exact-length check was refusing the entire head over it. That matches the shape this lane measured at H=1024 (Hunyuan-0.5B The fourth arm is the one I would have insisted on: a bound is only meaningful if the case just past it is pinned as a refusal, and the reason given ("the offsets are not established") is the actual justification rather than "too big". One question, not a blocker: why 2 and not 1? The only measured case is 1 slot, so the extra slot is provision for something unmeasured. If it is there for a family that pads two, naming it in the comment would make the bound a fact rather than a margin — and if nothing pads two, |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b0e4e44 — a relaxation with a bound, an example, and a reason it is safe; that is the only shape in which loosening an exact-length check is acceptable.
The case: Hunyuan-7B-Instruct 1.9 pads the head to whole 64-row tiles — 2004 slots for a vocabulary of 128,167 (2003 needed), i.e. 89 padding rows / 143,616 B — and the exact-length check refused the whole head over it.
Why accepting is safe, in the code's own words: the padding is whole tile-slots, so "the head's own offsets stay the ones this arithmetic derives", and it is small (slots ≤ 2) — anything else "means this is not this envelope plus this head and the offsets are not established". That is the correct two-part condition: alignment preserves the mapping, and a small bound keeps the acceptance from becoming "any tail longer than expected is fine".
return slots > 0 && slots <= 2; is the whole rule, and exporting it (fconst19_tail_padded) makes it testable without a pack — the same seam #1852's siblings use.
Not run: no pack on this box for the case (Hunyuan-7B), and the box is mid-build; the arm is host-only.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b0e4e44
The distinction is the right one and it is bounded rather than open-ended: a tail longer than the head by whole tile-slots (≤ 2) is vendor padding, while anything else means "the offsets are not established" and still refuses.
bool fconst19_tail_padded(int64_t env, int64_t rows, int64_t tile, int64_t have) {
const int64_t body = have - env;
if (body <= 0 || body % tile != 0) return false; // not whole slots -> not padding
const int64_t slots = body / tile - rows;
return slots > 0 && slots <= 2;
}
The % tile != 0 is what keeps the acceptance safe — "accepting is safe only while the padding is whole SLOTS (the head's own offsets stay the ones this
arithmetic derives)" — and the slots <= 2 cap keeps a genuinely mis-derived tail from being read as padding. Both bounds are necessary and both are
exported so they can be tested without a pack.
The arm covers exactly the boundary set, which is what I would have asked for:
exact -> not padded (the case that already worked)
+1 slot -> padded (the Hunyuan 2004-slot case: vocab 128,167, 2003 slots needed)
+2 slots -> padded (the cap)
+3 slots -> NOT padded (refused: beyond the cap)
exact + tile/2 -> NOT padded (the half-slot case, which is the dangerous one -- offsets would not line up)
I checked the arithmetic in the comment rather than taking it: 2003 slots × 64 rows = 128,192 ≥ 128,167 (so 2003 is the needed count and the last tile is
25 rows short of full), 2004 slots is one whole slot more, and the slot for that geometry is 64 rows × (kpad/2) B — consistent with "89 padding rows"
as 64 + 25. The numbers hold.
Built and ran: all npu-pack checks passed.
One note, not blocking: the tail-length check now accepts two different explanations (exact, or padded ≤ 2 slots) — when the head is eventually read, the
correlation is still what decides placement (that is #1773/#1789's argument), so this widens the window rather than weakening the gate. Worth keeping that
sentence in mind if the padding rule is ever loosened again.
|
Closing as superseded by #1773 ( What is worth keeping from this branch, if #1773 wants it: the six-arm predicate test. Mine exported Not re-opening: I merged main into this branch to resolve the conflict, saw |
…e slots (refs #1852) (#1869) #1789 taught this tool to place the head before a trailing zero run, which fixed the packs that pad the tail with BYTES. engine#1852 is the other shape: `Hunyuan-7B-Instruct` 1.9 needs 2003 tiles for a 128,167-row vocabulary and ships 2004, so the tail is 89 padding rows -- whole tile slots, not bytes -- and the #1789 rule lands a whole tile early on it. Two different paddings, one subtraction, and the file size alone cannot tell them apart. So the placement is now a list, grown backwards from where the payload ends (`size - zeros - (ntiles + k)*tile`, keeping only offsets at or after the envelope, bounded by a slack of 8 tiles the way the importer's own search is). With more than one candidate the tool probes tile 0 of each against the oracle and takes the best, refusing when nothing correlates or when two placements do; without an oracle it refuses outright rather than reporting the #1789 rule's answer as if it were measured. `geometry.placement` and `geometry.placements` say which one was used and how many existed. Measured: `Qwen2.5-0.5B-Instruct_rai_1.9.0_npu` has 9 candidates and placement 0 is the head (corr +1.0000, rel_err 0.0007 -- unchanged from before), which is the check that the search does not perturb the case that already worked. Four new checks pin the arithmetic, including the one that matters: a whole-slot pad gives two placements and the head is the SECOND (the envelope's end), i.e. the exact way the old rule was wrong on this pack. 20 checks pass. refs #1852 Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
import_packrequired a 1.9 pack's.fconsttail to be exactlyenvelope + rows × tileand refused the head by name otherwise.Hunyuan-7B-Instruct1.9 pads that tail to a whole number of 64-row tiles — a vocabulary of 128,167 needs 2003 slots and the pack carries 2004 (89 padding rows, 143,616 B) — so the head was refused on a pack whose offsets are exactly right:Measured on
amd/Hunyuan-7B-Instruct_rai_1.9.0_npu; attention 5120/5120 tiles and MLP 9216/9216 groups are byte-exact either way.The accept is deliberately narrow, and testable without a pack
Six arms, at the pack's own numbers (
env 8256, rows 2003, tile 143616)Mutation-checked: widening the bound to
slots > 0fails the +3 arm.Verified
Host-side, no device. Refs #1740 — the 4th pack of that board (Hunyuan-7B, h4096) imports completely with this, and its serve row now waits only on the dynamic-NTK rope rule in #1792.