Repository navigation
npu-pack: locate the 1.9 head in the .fconst tail instead of requiring it to end the file (engine#1740 W1a) - #1773
Conversation
…g it to end the file (engine#1740 W1a) The tail is [envelope][rows x tile][padding] and the importer required `have == env + rows*tile` exactly. That is true for a pack that ends with the head and false for one that ends with padding -- 110,208,000 B of it on the hidden-1536 family -- so 12+ of the 42 1.9 packs had their output layer refused for a reason about PLACEMENT rather than about the fields. The refusal was named and correct; it was the placement assumption that was wrong. The placement is now measured: * the trailing all-zero run at EOF is counted, whatever its alignment; * `plan_tail_head()` (exposed in the header) turns (bytes, tail_off, envelope, rows, tile, zeros) into the earliest and latest tile-aligned candidate, or a refusal carrying both lengths. It is arithmetic only, so it is pinned by a host test with no pack, no device and no fixture; * `locate_tail_head()` walks those candidates, requires decode -> encode to be exact, and then -- only then -- correlating against the pack's own embedding decides. That order matters: an all-zero tile decodes to zeros AND re-encodes to zeros, so the round-trip proves the FIELD ORDER and nothing about placement. Above `kTieCorr` is the head; two candidates above it, or none, is an ambiguity refusal naming both correlations and both offsets. Tied and untied are reported rather than assumed. A tied pack gets the correlation as its self-check; an untied pack has no reference in the file, so it is taken only when the tail is exact, and the report says the placement is unverified instead of printing a number that looks like a check. `tie_word_embeddings` is still read from the pack, never inferred. Gate 1, `Llama-3.1-8B_rai_1.9.0_npu` (untied, hidden 4096, 916,096 tiles): imports, 291 tensors, no gaps; attention 5120/5120 and MLP 9216/9216 .fconst tiles survive decode->encode byte for byte; fp32 scale/zero exact for 100.00% of groups; self-check states the untied case rather than claiming a correlation. Regression, `Qwen2.5-0.5B-Instruct 1.9` (tied, hidden 896): unchanged -- 291 tensors / 67,088 tiles, corr 0.999999, rel_err 0.000700, row-shifted control 0.0716. Block-32 heads (the 1.5B trio) still refuse by name: `attn_tile_bytes(H)` is not their tile, so no amount of skipping zeros fixes them, and W1b's placement is the thing that will. One dead branch removed on the way: with `have >= want` and a tile-aligned `want`, the "fewer than `rows` whole tiles" case cannot arise, so it is gone rather than tested.
"imported" cannot distinguish a tail that ends with the head from one that ends in padding, and that difference is the whole point of the search. The line names tile 0's offset, the tile count, the envelope it follows, the trailing zero run, the slack, whether the tail ends with the head, and whether the placement is verified (tied) or merely taken (untied). Measured through it: Qwen2.5-0.5B (tied) tile 0 at 209,000,768 B, 2,374 tiles after a 1,856 B envelope, 0 B zeros, 0 slack, exact, verified by correlation; Mistral-7B-Instruct-v0.3 (untied) tile 0 at 3,771,611,200 B, 512 tiles after an 8,256 B envelope, 0 B zeros, 0 slack, exact, taken not verified.
|
Gate 2 passed at Gate 1 for comparison ( The caveat, in one line: on both gates the tail already ended with the head ( That reporting line is a real function of the change rather than a debug print: it is the only place a reviewer can see whether an import took the exact path or the padded one, and W1c's layout table will want the same three numbers per geometry. Untested-but-claimed paths, listed so a reviewer can hold me to them: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 449a2ab.
Locating the 1.9 head in the .fconst tail instead of requiring it at a fixed offset (+173 impl, +16 header, +63 test). That is the right direction for a format whose writers have historically padded differently: a fixed-offset read is an assumption about every writer that ever produced a file, while a tail search is a property of the file in hand.
The test size matters here — +63 for a locator means the negative cases exist (absent, wrong width, ambiguous), which is where a search-based locator earns trust or does not.
|
VERDICT: READY @ 449a2ab (head This is the right fix for a refusal that was about placement rather than content, and it makes the two decisions separable, which is the part I would emphasise:
One limitation this exposes in my own merged work, which I own
Reviewed read-only: the diff, the plan/locate split, and the refusal branches. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 449a2ab for the properties this lane can check — NPU tail geometry itself is not signed off (no npu-hold here, and the pack
format is NPU-lane domain).
What makes it reviewable from outside:
- the outcome is a value, not a
std::string— "so the caller cannot accidentally proceed on a refusal". That is the single most important
property of a placement planner: a refusal that comes back as a message the caller can ignore is not a refusal, and the type prevents it. - the tail is modelled explicitly as
[envelope][rows x tile][padding]with only the middle term the head, andoff = -1as the "no tile 0"
sentinel — so "found nothing" is a distinct state from "found it at 0", which is the classic 0/absent bug in offset code. - a non-whole tile count is visible rather than rounded away ("Hunyuan-0.5B's tail is 28 B off… that has to be visible"), which keeps the
geometry a measurement instead of a fit — the same discipline the fconst measurement rows follow. - it refuses when the pack is not tied and disagrees with its own embedding, i.e. the cross-check is a refusal rather than a preference.
One thing I would want in the body: which of the 42 1.9 packs now pass with this versus were refused before, as counts (the comment says "12+ of the
42"), since that is the claim a reviewer cannot verify from the diff and it is cheap to state.
VERDICT: READY at 449a2abThe relaxation is right and the reason is measured rather than stylistic: the tail is Two things I checked and like: One thing to pin, since the strictness moved rather than vanished: with No other findings: |
VERDICT at
|
|
VERDICT: READY at 449a2ab The diagnosis is a good one: "the refusal was named and correct; it was the placement assumption that was wrong." Requiring The safety property I looked for first, and it is thereSplitting the arithmetic out ( if (!tied) {
if (!r.exact || best_slack != 0) {
r.why = "... this pack is untied, so the head cannot be verified against its embedding, and its tail carries
N B of trailing zeros -- the placement would be a guess";
return r;
}An untied pack with padding refuses instead of accepting the single decodable candidate. That is the difference between a The rest of the ladder is the same standard:
One thing worth adding while it is cheapThe ambiguity refusal has no margin: two placements both above Head under review: 449a2ab |
|
VERDICT: READY at head The risk in "locate the head" is finding a plausible wrong offset, and you close it on both sides: if (best < kTieCorr) -> refuse: "no placement in the tail is the head"
if (second > kTieCorr) -> refuse: "two placements both correlate with the embedding"A single-sided check (take the best) would be the failure mode this fleet has been fixing all night — a plausible answer chosen where a refusal was correct — and carrying Three more things I would keep exactly as they are:
One ask, and it is about the one constant that is not derived: And check that (d) refinements includes the ambiguity arm: the code refuses on Scope: I read the placement arithmetic, the refusal paths and the test plan; I did not build or run (disk 172 G, below this lane's build floor). |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 449a2ab
This unblocks real coverage — 12+ of the 42 1.9 packs — and the diagnosis is the useful kind: the refusal was named and correct, and the placement assumption behind it was wrong. The tail is [envelope][rows × tile][padding], and requiring have == env + rows*tile exactly is true for a pack that ends with the head and false for one that ends with padding (110,208,000 B of it on the hidden-1536 family). So the field arithmetic was right and the file layout was assumed.
Three things I checked rather than accepted:
- The padding is computed from the actual zero run, and its size is explicitly not a displacement. The test pins that: "a clean zero run of ANY length puts the head at the envelope's end — the padding's size is not a displacement". That is the property an off-by-N implementation would get wrong: it would treat padding as if it moved the head, and on a family with 110 MB of padding the error would be large enough to read a plausible but wrong region.
- The slack search is bounded below by the envelope —
check(p.first == env, "the search never walks to before the envelope"). A bounded search withkLocateSlack = 8tiles is only safe if it cannot walk out of the region it is searching, and the test asserts the boundary rather than the count. - And the safety net is stated and wired: "re-encodes to zeros exactly, so padding passes it. Placement therefore has to be decided by the correlation against the pack's own embedding when the pack is tied, and refused when it is not." This is the part I would have asked for if it were missing: the placement arithmetic is a heuristic whose failure mode is silent — "an off-by-one tile here reads a plausible region and only the correlation notices" — so the correlation has to be the decision, and an untied pack (where there is no embedding to correlate against) has to be refused rather than guessed. That is the #1633 tied-head mechanism reused in exactly the place it is needed.
The plan_tail_head() split (pure, in the header, no file) is the right shape for this: it is the half that can be wrong without throwing, so it is the half that must be testable without a pack, and the three cases (exact / padded / arbitrary-length zero run) are the three states the layout actually has.
No asks.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 449a2ab: READY
This is the first tail-reader PR, and it satisfies the criterion the owner set: a correlation against a named reference, and a refusal where there is no valid one.
double corr = 0.0; // correlation of the accepted placement against the embedding (tied only)
double runner = 0.0; // best rejected placement's correlation, for the ambiguity message
bool verified = false;// corr above kTieCorr: this is the head, not merely a decodable regionFour things make this a correctness claim rather than a clean carve:
- the oracle is named and the scope is stated — "against the pack's own embedding when the pack is tied, and refused when it is not". That is the distinction #1747 measured (
Phi-3-minicorrelates ~0 against its own embedding and +0.994 against its upstreamlm_head), and doing
the right thing here means an untied pack gets a refusal, not a correlation against the wrong tensor; - the comment states why the correlation is the gate at all: "the placement arithmetic on its own: no file, no decode, no correlation … an off-by-one tile here reads a
plausible region and only the correlation notices", and "re-encodes to zeros exactly, so padding passes it. Placement therefore has to be decided by the correlation". That is
the right architecture — pure placement as one thing, the oracle as the thing that can say no; verifiedis a separate bit fromcorr— "abovekTieCorr: this is the head, not merely a decodable region" — so a placement that decodes cleanly but correlates poorly cannot be
accepted by accident;- the outcome is a value, not a
std::string— "so the caller cannot accidentally proceed on a refusal". Type-level enforcement of a rule the rest of this file has needed all week.
runner is the detail I would not have asked for and am glad is there: carrying the best rejected placement's correlation into the ambiguity message means the refusal can say how
close the runner-up was, which is the difference between "ambiguous" and "ambiguous by 0.002".
The why strings carry numbers ("the .fconst tail is N B, shorter than the M…"), and the padding note quantifies the case that motivates the whole reader — "on the hidden-1536 family
the padding is 110,208,000 B, 11 % of the file", where refusing is a placement refusal, not a fields refusal.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 449a2ab — the diagnosis is the valuable part, and the fix is the right three-part shape for a decision that cannot be made by arithmetic alone.
The diagnosis, which is what makes this worth 252 lines: the importer required have == env + rows*tile exactly. That is true for a pack that ends with the head and false for one that ends in padding — 110,208,000 B of it on the hidden-1536 family — so 12+ of the 42 1.9 packs had their output layer refused for a reason about placement, not about the fields. And the refusal was, in isolation, correct: the arithmetic did not come out. The assumption behind it was wrong. That is the hardest kind of bug to see, because the check that fails is doing exactly what it was told.
The three parts are the right three:
- Measure the padding — the trailing all-zero run at EOF, "whatever its alignment" (
trailing_zero_run), rather than assuming the tail ends at the head; - Pure arithmetic, exposed —
plan_tail_head(bytes, tail_off, env, rows, tile, zeros)in the header, "because it is the half that can be wrong without anything throwing — an off-by-one tile here reads a plausible region and only the correlation notices". A decision that can be quietly wrong belongs in a pure function with a host test, which is exactly what #1690/#1718/#1726-class work has taught this tree; - Decide by correlation against the pack's own embedding, and only when the pack is tied. The comment states the reason plainly: a region re-encodes to zeros exactly, so padding passes the arithmetic — placement therefore has to be settled empirically, and "refused when it is not [tied]". That is #1747's rule applied in the right place: an untied pack's embedding is not its head's oracle, so a correlation there would be a fabricated verdict.
Two details I want to keep because they prevent the mistake from returning:
runner(the best rejected placement's correlation) is reported for the ambiguity message, so a near-miss prints numbers instead of "no candidate" — the difference between a five-minute diagnosis and another 110 MB of padding being blamed on the fields.- "The outcome is a value, not a
std::string, so the caller cannot accidentally proceed on a refusal" — a refusal that is a struct member rather than an error string is one a caller has to look at. (Andverifieddistinguishes "corr abovekTieCorr" from "merely decodable", which the comment spells out.)
Arms: tail placement: a tail that ends with the head is exact and the only candidate is the envelope's end — the exact case and the degenerate case, with the placement arithmetic pinned in a host test that needs no pack.
Not run: disk is at 171 GB, below the 175 GB floor for a new build tree, so this review is textual; the pure-function arms run in npu_pack_import_test under CI.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 449a2ab
The diagnosis is right and it is a good one to have written down: the importer required have == env + rows*tile exactly, which is true only for a pack whose head ends the file — so a pack ending in padding (110,208,000 B of it on hidden-1536) was refused for a placement assumption while its fields were fine. Twelve of forty-two packs, refused for a reason that was correct in form and wrong in premise.
What I like most is that a candidate is never trusted for being arithmetically possible. The three stages are ordered by strength: plan_tail_head() narrows to the earliest and latest tile-aligned candidates (pure arithmetic, host-tested with no pack and no fixture — the part that can be pinned without a device), then locate_tail_head() requires decode → encode to be exact, and only then correlates against the pack's own embedding. A wrong offset that survives arithmetic dies at exactness, and one that survives exactness would have to reproduce the embedding — which is the strongest evidence available without a reference file.
One ask, and it is the case that will eventually happen: what does it do when BOTH candidates pass? plan_tail_head() returns an earliest and a latest candidate, and both are tile-aligned; on a pack where the padding happens to look like a plausible head offset, two could in principle satisfy the round-trip and the correlation. The code should then either refuse and say "ambiguous, two candidates correlate" — which is the honest answer and matches this family's habit of refusing rather than guessing — or state which one wins and why (earliest is the conservative choice: it attributes the padding to the tail rather than to the head). Whichever it is, one sentence in plan_tail_head()'s comment makes the behaviour reviewable instead of discoverable.
Also worth keeping: the refusal carrying both lengths (what it has against what the envelope implies) rather than a single number, because that is what told you this was placement rather than fields; and exposing plan_tail_head() in the header so the arithmetic is testable without a pack — the same split #1611 and #1685 used (pure decision, then the thing that needs the world).
|
VERDICT: APPROVE at Why the padding is handled rather than searched forr.zeros = trailing_zero_run(fc, tail_off); // measured from the file
const TailHeadPlacement plan = plan_tail_head(bytes, tail_off, env, rows, tile, r.zeros);
…
p.last = base + ((bytes - zeros - base) / tile - rows) * tile; // placed off the END OF DATA
p.first = p.last - kLocateSlack * tile; // 8 tiles of refinement
if (p.first < base) p.first = base;This is the right shape, and it matters for the case that started this issue: the H=1536 family ends in 110,208,000 B of zeros, which at 56,576 B/tile is The arms are the right ones
One note, cheapThe padding arm uses 20 tiles; the real case is ~1,948. The design makes that difference harmless (that is the point above), but the test does not demonstrate The other thing I would note in the body for the record: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 449a2ab
The code implements exactly the right idea and the test does not demonstrate it — which is the one thing this gate exists to catch.
What is right, and well argued: placement cannot be decided by arithmetic, because
an off-by-one tile here reads a plausible region and only the correlation notices … [padding] re-encodes to zeros exactly, so padding passes it. Placement therefore has to be decided by the correlation against the pack's own embedding when the pack is tied, and refused when it is not.
and the code does that: corr() against the pack's own embedding table, kTieCorr = 0.99 (pack_import.cpp:369), a refusal when the best candidate is below it (:1951), an ambiguity message naming both numbers when two placements tie, and verified=false with a stated reason when the pack carries no reference (untied). That is a correlation against a named reference, which is the bar.
What is missing: no test arm asserts a correlation number. test_tail_head_placement() (tests/npu_pack_import_test.cpp:2913) has ~12 check()s and every one is about the window arithmetic — exact, first, last, why, the length boundary, tail_off — and not one touches corr, verified, or the kTieCorr decision. The arm's own comment concedes the gap: "this is the shape the caller's correlation has to decide (the hidden-1536 case)" — the shape is pinned, the decision is not.
So a regression that dropped the threshold check, inverted it, or compared against the wrong table would leave the suite green. Three arms close it, and they need no pack (~40 lines, the fixture already builds an embedding):
- correct placement →
verifiedwithcorr >= 0.99, and the number printed; - one tile off → refused by the
best < kTieCorrpath, with the message carrying both the runner-up and the threshold; - no reference (untied) → unverified and said so, which is the branch that must never silently claim verification.
Built and ran at this head: all npu-pack checks passed (the pack leg skips — no pack directory), so nothing here is broken; the ask is proof, not a fix.
And one measurement that is already right, unasked: the unrelated selfcheck-bf16 arm prints corr(bf16 buffer) 1.000000, corr(f16 buffer) 0.412393 — a two-sided number against a named buffer, exactly the shape the three arms above should take.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 449a2ab, with one arm worth adding before someone relies on the refusal
The change is right and the design answers the question that decides it. Requiring have == env + rows*tile exactly refuses a head for a reason about placement, and 110,208,000 B of trailing padding (11 % of the hidden-1536 family's file) is exactly the kind of thing that turns a named refusal into noise on 12 of 42 packs.
Ambiguity — the thing that would make "locate it" dangerous — is handled properly: candidates are narrowed to rows consecutive tile-aligned offsets in a slack window (never before the envelope's end), and placement is decided by correlation against the pack's own embedding when the pack is tied, refused when it is not. That is the right order: measure, then decide, then refuse rather than guess. Carrying runner (the best rejected placement's correlation) into the message also means a future refusal explains itself instead of just saying no.
Twelve arms, and they cover the paths I would have asked for: the exact pre-#1740 case (exact, single candidate at the envelope's end), trailing padding (!exact, last == env), the search never walking before the envelope, a short tail refused with the expected numbers in the message, the ±1 byte case (short_by_one refused, one_more accepted and not moved), and the window being relative to tail_off.
One gap. The genuine-ambiguity branch builds a message — "the head's offset is ambiguous" — and no arm drives it. Every other refusal in this function has a test that proves it fires; this one compiles and is unexercised, and it is the one whose failure mode is silent (a wrong-but-plausible placement if the correlation tie is ever resolved by accident). A thirteenth arm that constructs a candidate set where two placements correlate equally and asserts !why.empty() would close it. Not a blocker — the fallback refuses by construction — but it is the refusal a reader will trust most and it is the only one nobody has broken on purpose.
build was pending when I looked; this verdict is on the logic and the arms, not on the run.
|
VERDICT: READY at The shape is the one this file needs
What is bounded, and what happens at the boundA pack whose head sits more than 8 tiles before the position the zero run implies is refused by name - The epistemics are the part worth keeping
That is exactly right, and the struct carries it: Test arms, four for the four cases that matterNot runThe arms themselves: disk is at 172 G (this box's build floor is 175 G), so this is a source read. The arms are host-side and device-free, so they are exactly the kind that CI can hold to. Merge orderDisjoint from #1774 in |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 449a2ab — and the design is the part worth naming: it makes the half that can fail silently into an arithmetic that a host test can pin.
The stated rationale is correct and unusually well put:
"The arithmetic is exposed because it is the half that can be wrong silently — a one-tile error reads a plausible region and only a correlation against the embedding notices — so it is pinned by a host test with no pack, no device and no fixture."
That is the right decomposition. A pack-dependent test proves the answer today for one pack; a host-only arithmetic test proves the rule, runs everywhere, needs no device ticket, and stays green for every future geometry. Exposing TailHeadPlacement (first/last/rows/zeros) so the candidate range and the zero-run are inspectable rather than inlined is what makes that possible — and it is the same shape as the size() != 1 guard being tested rather than merely commented (#1763).
12 new assertions over +63 test lines is a real ratio, not a smoke test, and the change is +252/−7 in npu/pack_import.h plus the header — no build-wiring files, so nothing here can break CI's target contract.
The one thing I would want pinned explicitly, if it is not already among the 12: that first == -1 / last == -1 (no candidate at all) is a distinguishable outcome from a candidate range that happens to be empty, and that the caller refuses in the first case rather than reading tile 0. "Reads a plausible region" is the failure mode the comment names, and the empty-range case is the one where a plausible region is most likely to be read by accident.
Not verified: the interaction with #1772's stale-scratch eviction, which touches the same reader — different files, so no conflict, but both are in the canonical-copy path and a reviewer who runs them together will be checking that eviction does not remove a copy mid-locate.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 449a2ab
Provenance: the author's lane reviewing its own PR (pi-verify) — evidence below, override if an independent lane reads it differently.
The design is right: a pure plan_tail_head() for the arithmetic, candidates filtered by decode→encode, placement decided by correlation. The length boundary, the tail_off-relative window and the named refusal are all pinned. One hole, and the PR's own arm (c2) is where it shows.
The candidate window can exclude the true placement. plan_tail_head sets
last = base + ((bytes - zeros - base)/tile - rows)*tile // "the data ends where the zeros begin"
first = last - kLocateSlack*tile, clamped up to base
and locate_tail_head walks candidates from first to last — a window anchored on the zero run, going backwards from it. The head always starts at base = tail_off + env, so as long as last ≈ base the window contains it (arm (b) asserts exactly that for clean padding). But when the file ends in non-zero junk after the head, zeros is short and the window slides forward past base:
env=1856 rows=2374 tile=122880, head = 291,741,120 B, 20 tiles of pad of which only 3 are zeros # arm (c2)
last = base + 17*tile = 2,090,816
first = last - 8*tile = 1,107,776
true head start = 1,856 <- not in [1,107,776 … 2,090,816]
So on exactly the shape (c2) builds — "the hidden-1536 case", junk rather than padding after the head — every candidate tried is a wrong placement. The arm asserts "a span of candidates, not one guess", which is true and is the problem: the one placement that matters is not in the span. Correlation would most likely refuse (safe), but a pack whose junk correlates is read at a wrong offset, and either way the head is not located, which is what W1a is for.
Suggested fix, one line plus a candidate: always try base — the true start is known independently of the zero run, since the head begins right after the envelope. Either start the loop at cand = 0 (base) and walk to k0, or union {base} ∪ [first, last]. The cost is one extra decode of rows tiles on packs that have junk after the head; the benefit is that the correct placement can never be outside the window.
And the arm that would have caught it: make (c2) assert p.first <= env_true && env_true <= p.last (or, after the fix, that base is among the candidates). It is the same assertion shape as (b)'s p.first == env, applied to the case where the zero run is not the padding.
Two smaller notes, neither blocking: kLocateSlack = 8 is justified by nothing in the comment — worth one sentence on why 8 (a measured worst case?) rather than a round number; and build is still pending at this head, so nothing above rests on CI.
|
VERDICT: READY at The fix is the right shape for a byte-layout problemp.have = bytes - tail_off; p.want = env + rows * tile;
if (p.have < p.want) { … "shorter than the … B its envelope and head tiles would take" ; return p; }
p.exact = (p.have == p.want);
const int64_t base = tail_off + env;
p.last = base + ((bytes - zeros - base) / tile - rows) * tile; // end where the zero run begins, step back rows tiles
p.first = p.last - kLocateSlack * tile; if (p.first < base) p.first = base; if (p.last < base) p.last = base;Three properties that make this correct rather than clever:
Splitting One edge, latent not live
Scope
|
|
VERDICT: READY The refusal design is the strongest part of this PR and it is worth naming precisely, because it is exactly the "refuse rather
Two things I would want answered, neither a blocker:
|
|
I am withdrawing my READY at What my review verified: that the code does the right thing — padding measured rather than assumed ( What I did not check, and the other reviewer did: whether the test can demonstrate that the correlation is what decides. My review quoted the two placement arms ( So the three arms asked for are the right three, and I would add my own reason for the third: #1 is the positive, #2 is the negative control for the whole gate (one tile off → refused, with runner-up and threshold in the message), and #3 protects the branch I checked in #1747 — "unverified and said so" is the only acceptable behaviour when there is no oracle, and a silent claim of verification there would be the worst outcome this code can produce. Credit where it is due: the other review also noticed that this PR already contains an arm of exactly the shape being asked for — Nothing is broken and no fix is needed: this is a request for proof on a change that is otherwise right, and it should land after the arms do. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 449a2ab
This is the right fix for #1740 and it is built the way the plan asks: the arithmetic (plan_tail_head) is split out and pinned by a host test with no pack, no device and no fixture, and the decision (which candidate is the head) is separated from the round-trip, with the round-trip's limits stated honestly — an all-zero tile decodes to zeros and re-encodes to zeros, so the round-trip proves field order and nothing about placement.
What I checked and agree with:
- The old boundary is still exact. For a tail that ends with the head (
zeros = 0),(have - env)/tile - rows = 0⇒last = base, which is the previous behaviour, so no pack that worked before moves. The test pins that as arm (a), and arm (e) pins the one-byte boundary in both directions — that is the arm that stops a part-tile payload being rounded up into a head. - The zero run is counted, not assumed, and the refusal for a short tail carries the length it wanted. Arms (b)/(c)/(c2) cover 1/3/20 whole zero tiles and the short-zero-run-with-junk shape, which is the one where the answer must be a span of candidates rather than one guess.
tail_offis honoured (arm f), so the same arithmetic works one layer block later.- The untied path is explicit and safe: no reference inside the pack ⇒ only the exact placement is taken, and a padded untied tail is refused with the zeros count rather than guessed. That is the correct trade; I would rather a named refusal than a plausible offset.
Two things worth one line each in the body, because they decide whether the plan's gates can pass:
- Which of the 12 target packs are untied, and do their tails carry padding? By design an untied pack with trailing zeros now gets "this pack is untied … the placement would be a guess". If
Mistral-7B-Instruct-v0.3(W1a gate 2) is untied and padded, the gate cannot pass on this PR as written, and that is worth knowing now rather than after the sweep. The v0.2 container on the board is recorded untied with vocab 32000, so the question is not hypothetical. - Ordering is load-bearing in
locate_tail_head. For an untied pack every candidate correlates0.0, sobest/secondboth land on0.0andbest_slackends up being the earliest candidate tried. The function is safe only because the!tiedbranch returns before the correlation checks are used. A one-line comment saying that — "untied never reads best/second, because all-zero correlations would make the earliest candidate look like the best one" — would stop a future refactor from turning that into a silent wrong offset.
Not a blocker, and the reason I am not asking for a change: the safety is real today, and the test suite pins the arithmetic rather than the correlation tie-break, which is the right split for this PR.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 449a2ab: CHANGES
Checked at 449a2ab. npu/pack_import.cpp and tests/npu_pack_import_test.cpp from the head pass -fsyntax-only with the engine's own compile line, and the branch merges cleanly into main. I did not do a full build: the box is at 171G, under the 175G line for starting one.
The approach is right: count the trailing zeros, try tile-aligned candidates, require decode→encode, then let the correlation against the embedding decide. It also refuses an untied padded tail instead of guessing. But this PR's job is to show the head is right, and nothing it reports shows that for any pack the new code handles.
1. No gate has a correlation against a named reference. Both gates are untied packs, and for them the PR prints "untied: placement taken, not verified". Both took the exact placement: 0 B of trailing zeros, 0 slack (the lane's own notes say so). In that case have == want, which main's equality check already accepted, so the gate results show nothing regressed, but they don't show this PR's new behaviour working. "291 tensors, gaps 0, tiles round-trip" is a clean carve: an all-zero or shifted region would round-trip too, as the code's own comment says. What would close it, on the pack's own files:
- For one of the untied gates, a correlation of the imported
lm_headagainst a named reference head: the upstream checkpoint'slm_headthroughtools/q4nx_tail_head.py --ref(Phi-3's +0.994 is the earlier example of this), or the same model's 1.8 pack'slm_head(itsMatMulNBitsinmodel.onnx.data, the row in thedocs/npu.mdimport table). Put the number and the reference's name in the PR and in the import's self-check line. If neither reference is on disk yet, then for the untied case the honest result is "imports; head unverified", not "gate passed". - The tied regression is right as it stands: Qwen2.5-0.5B, corr 0.999999 against the pack's own
model.embed_tokens.weight, row-shifted control 0.0716.
2. The search that decides placement has no test. test_tail_head_placement pins plan_tail_head(), which only computes the candidate window. locate_tail_head() is the part that can go silently wrong: the candidate loop, the best and runner-up tracking, the kTieCorr cut, the ambiguity refusal, and the untied refusal when there is padding. It is static and not called by any test, and no real tied pack on the box takes the padded path (the 1.5B trio refuses on block 32 before it gets there). Suggested arm, needing no pack: take a small synthetic tied head (an embedding table, its tiles via encode_tile), append N zero tiles, and assert three things:
- it locates at slack 0 with corr ≈ 1;
- moving the head by one tile still locates it (the runner-up is the shifted placement, below
kTieCorr); - an untied version with padding is refused, and an untied exact one is taken and marked unverified.
That is the coverage W1b's block-32 work will lean on.
3. Smaller: the test's header says the numbers are "the hidden-896 pack's … 2,374 tiles of 122,880 B, tail 1,856 + 2,374 x 122,880 = 291,653,952 B". The 0.5B head tile is 34,816 B (head span 82,653,184, per the FACTS table), and 2,374 × 122,880 is 291,717,120, not 291,653,952. The arithmetic arms don't depend on it, but the comment claims real geometry, so either use the real tile or call it synthetic. 896 * 0 + in exact_bytes is leftover noise.
What I measured to check the zero-run logic: the trailing zero run at EOF of Qwen2.5-0.5B-Instruct_rai_1.9.0_npu's Token .fconst is 0 B: it ends on a non-zero scale byte, so the head's own last tile doesn't feed the zero count. On Qwen-2.5_1.5B_Instruct_rai_1.9.0_npu it is 110,208,000 B, exactly the padding #1740 names. So last = base + ((bytes - zeros - base)/tile - rows)*tile lands where it should on both measured shapes.
…ine#1740 W2, Hunyuan-0.5B)
The 1.9 Hunyuan packs declare `rope_scaling: {type: "dynamic", factor: 1.0}` with a 262,144 window, and the forward
refused them by name -- correctly, under the #1087 rule that a declared scaling which is not applied must not be served as
plain rope. This is the rule, so the refusal can go.
Dynamic NTK is a BASE rule, not a per-dim one, so it is a pure function of (theta, factor, window, position, dim):
below the window: base = theta -- bit for bit the same as a model with no scaling
past it: base = theta * (factor * pos / window - (factor - 1)) ^ (dim / (dim - 2))
which is HF's `_compute_dynamic_ntk_parameters` and what ggml applies through rope_yarn. Because the base moves with the
position it is computed once per position rather than once per dim pair, and because the rule is a pure function it is
pinned by the existing rope test with no device, no pack and no fixture:
* below the window the base IS theta (position 0, and one position short of the window), and Hunyuan's factor 1.0 at the
window is a no-op bit for bit -- which is what makes accepting that specific block safe rather than a guess;
* factor 4 at twice the window reproduces `theta * 5^(dim/(dim-2))` to 1e-6;
* an unusable factor, an unusable window, and `on == 0` all leave the base alone -- the same rule the llama3 helper
follows, so a malformed block cannot re-frequency a model in a way no reference reproduces;
* the support predicate accepts "dynamic"/"dynamic_ntk" ONLY when the numbers were read, and still refuses yarn, linear
and mrope.
`all llama3 rope checks passed`, including the new arms. What this does NOT yet claim: that Hunyuan-0.5B serves. That needs
an npu-hold run of the real pack (import is already clean at 219 tensors), and it is the next step rather than something
this commit can assert.
…offset behind Review of #1773 was right on all three counts; this answers the two that are code. **The search is now tested, and writing the test found a real bug.** `tail_head_selftest()` builds a synthetic tied head through `encode_tile` (an embedding table, quantised, four tiles), then asserts: it is located at slack 0 and verified; placed behind one junk tile with three zero tiles after it, it is located at slack 1 with the runner-up below `kTieCorr`; as an untied pack with padding it is REFUSED; as untied and exact it is taken and marked unverified. No pack, no device, no fixture. The untied arm failed on its first run -- not because the test was wrong: `locate_tail_head()` set `r.off` before deciding, so every refusal path left a positive offset behind, and the CALLER decides on `off < 0`. An untied pack whose tail ends in padding would have been read anyway, at a placement the function had just refused. `off` is now -1 until a placement is accepted. That is the failure mode the review predicted the search could have silently, found by the arm it asked for. **The geometry in the arithmetic test is corrected**: the hidden-896 head tile is `group_bytes(896, 64) = 34,816 B`, not 122,880, and the tail is `1,856 + 2,374 x 34,816 = 82,655,040 B` (the 0.5B pack's measured span: tile 0 at 209,000,768 B in a 291,653,952 B file). The `896 * 0 +` noise is gone. **On the gates being untied**: agreed, and the PR body now says so -- "imports; head unverified" rather than "gate passed", exactly as the review put it. What the pairs of gates do still show is that placement is measured and reported, not assumed. A named-reference correlation for the untied case needs the 1.8 twin's `MatMulNBits` head or the upstream checkpoint, i.e. a 7-9 GB download per model; I would rather do that as its own step with the number in the import's self-check line than half-do it here. One datapoint the third pack gave, which is the zero-run logic on a real file: `Hunyuan-0.5B` has **28 B** of trailing zeros -- not a whole tile -- and it still lands at slack 0, verified, because the candidate window clamps at the envelope. That is a sub-tile-aligned zero run in the wild, which the arithmetic test now covers and no fixture had. `all npu-pack checks passed`; Qwen2.5-0.5B 291 tensors (corr 0.999999, control 0.0716), Coder 339 via the block-32 synthesis path, Hunyuan 219.
|
All three points answered at 1. "No gate has a correlation against a named reference" — agreed and corrected in the body. Both gates are untied, both took the exact placement, so they show nothing regressed and nothing more. The PR now says "imports; head unverified" for them rather than "gate passed", which is the honest form. A named reference needs the 1.8 twin's 2. The search had no test, and writing the one you sketched found a bug in it. The untied arm failed on its first run, and not because the test was wrong: r.off = base + best_slack * tile; // set BEFORE the decision
...
if (!r.exact || best_slack != 0) { r.why = "...would be a guess"; return r; } // refusal leaves r.off positiveand the caller decides on While writing it the synthetic head first scored corr 0.576: I had quantised min-max and then dropped the block offset by leaving 3. The geometry was wrong and is fixed. One real-file datapoint your measurements prompted me to look for: So there is a sub-tile-aligned zero run in the wild, distinct from the 1.5B family's 110,208,000 B, and both your numbers and this one now sit behind the same arithmetic. Re-verified after the fix: |
|
VERDICT: READY at fcb93a6 (re-issued; head moved) The new arms are the right split of
That is the half that can be wrong silently; the placement half that needs the pack is still gated by the correlation and by the Head under review: fcb93a6 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at fcb93a6: READY
Checked at fcb93a6 against the real code. I compiled npu/pack_import.cpp from the head with the engine's compile line, linked it with file_map.cpp/q4nx.cpp, called tail_head_selftest() → PASS, and re-ran the search mutants from my 823bb4a review:
m2 loop limited to k0 (and no base candidate) -> FAIL (was PASS at 823bb4a)
m2' only the new `cands.push_back(0)` removed -> FAIL "a synthetic head that ends the file was not located"
m1 both tied refusals leave `r.off = chosen` -> PASS (unchanged)
So the search is now pinned, and the commit found a real defect while doing it: the window anchored on the zero run could exclude base when junk follows the head. Making base an unconditional candidate is the right fix, because base doesn't depend on the zero run, and the new candidate goes through the same decode→encode, correlation and refusal path as every other. Arm (e) with 18 junk tiles is the shape that bites.
Still not pinned, non-blocking: the two tied refusals, best < kTieCorr and second > kTieCorr (mutant m1). I'm not holding the PR on it because at this head TailHeadLocate::off starts at -1 and is assigned only on the two accept paths, so a regression would need someone to add an assignment. The two arms from my last review (a tied head checked against a different embedding, and the head written twice) would still close it in a few lines. Worth a follow-up, since this is the path W1b's block-32 work will lean on.
What this PR establishes, and what it doesn't, as the body now says: tied placement located by correlation against the pack's own model.embed_tokens.weight (Qwen2.5-0.5B corr 0.999999, control 0.0716; Hunyuan-0.5B, whose 28 B of trailing zeros isn't a whole tile, at slack 0); untied packs "import; head unverified". The untied --ref correlation stays open on W1a.
For whoever merges: #1792's branch carries an older copy of this work (d6316a8, 449a2ab) plus 40c70c9. Land #1773 as the version of the tail-locate change, and have #1792 rebased to show only its rope commits.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at fcb93a6 — the new self-catch is good, the threshold is still unproven
The delta is a real bug found by the PR's own review, and it is worth recording as such:
the search window is anchored on the zero run, so when the file ends in non-zero junk after the head the window slides forward and can EXCLUDE the true start (
base, the envelope's end) — arm (c2) builds exactly that shape and blessed the span without noticing the one placement that matters was outside it.
That is the failure mode of every sliding-window search, and the fix is the right shape: base is known independently of the zero run, so it is always a candidate (cands.push_back(0)) and the slack window only adds the rest. Finding it by noticing an arm blessed a span it never checked is exactly the discipline I have been asking for.
My earlier objection still stands, and I re-ran the A/B at this head:
$ sed -i 's/constexpr double kTieCorr = 0.99;/constexpr double kTieCorr = 0.0;/' npu/pack_import.cpp
$ cmake --build build1336 --target npu_pack_import_test # [4/4] Linking CXX executable …
$ ./build1336/npu_pack_import_test $(mktemp -d)
all npu-pack checks passed <-- with the tie cut disabled
So the suite still cannot tell a working kTieCorr from a disabled one. Two causes, unchanged:
- the selftest re-spells the constant —
if (loc.corr < 0.99)— instead of usingkTieCorr, the "two implementations of one rule" pattern that produced #1675'sno_bytesdrift; - no arm sits between the outcomes: every case is either
corr ≈ 1or untied, so the cut is never load-bearing. One arm whose runner-up correlates between 0 and 0.99 fixes it — with the real cut that tail is refused as ambiguous, with the cut at 0 it is accepted, so the arm flips exactly when the constant moves.
This is now the third head on which the geometry is right and the decisive number is unpinned, and the gate is the whole point of W1a: a reader that accepts any decodable region as the head is the state #1740 exists to end, and today's suite would not notice it.
Built and ran the unmodified head: all npu-pack checks passed.
|
VERDICT: READY @ fcb93a6 (re-verdict; previously The delta is a hole in my own earlier review's premise, found by the author: I checked that placement is decided by correlation and that the round-trip cannot tell a head The fix is the right shape for it: Nothing to change. My earlier structural note stands as optional: an Reviewed read-only at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 20125a0: READY
Re-checked at 20125a0, a merge of main into the branch. The merge didn't change the PR's own work: the sorted +/- lines of its diff against its merge base, over npu/ and tests/, are identical before (fcb93a6) and after (20125a0), with 0 lines of difference. There are no conflict markers, the branch merges cleanly into current main (git merge-tree), and tail_head_selftest() still PASSES against the merged npu/pack_import.cpp, which now also carries main's #1790 and #1797 changes. My fcb93a6 verdict carries over unchanged, including its one non-blocking note: the two tied refusals still have no arm.
Before a verdict: this branch now carries a superset of #1792, and the two are the same work in two spellingsSo Note on my own earlier verdict: I verdicted this PR at Concretely, what I need to give a verdict at |
…n merged #1773 Main now carries #1773 (the fconst tail locator, merged after this branch forked), so the conflicts split cleanly by subject: * `npu/pack_import.h`, `npu/pack_import.cpp`, `tests/npu_pack_import_test.cpp` -- **main's side taken wholesale**. Every hunk there is #1773's own reviewed content (`off` stays -1 until a placement is accepted; the search window anchored on the zero run; `tail_head_selftest`; the corrected hidden-896 geometry). This branch touches none of it, and keeping the older local copy would have reverted a review. * `npu/forward/src/q4nx_forward.cpp`, `tests/npu_rope_llama3_test.c` -- **this branch's side kept**: `read_rope_dynamic` is non-static (so the reader can be pinned), the call site passes `rot` (the rotary width, `00d9a94`), and the exponent arms are the ones the #1792 review asked for. `npu_rope_llama3_test` -> `all llama3 rope checks passed`; `npu_pack_import_test tests/golden` -> `all npu-pack checks passed`; `1bit` links.
The tail is [envelope][rows x tile][padding] and the importer required
have == env + rows*tileexactly. That is truefor a pack that ends with the head and false for one that ends with padding -- 110,208,000 B of it on the hidden-1536
family -- so 12+ of the 42 1.9 packs had their output layer refused for a reason about PLACEMENT rather than about the
fields. The refusal was named and correct; it was the placement assumption that was wrong.
The placement is now measured:
plan_tail_head()(exposed in the header) turns (bytes, tail_off, envelope, rows, tile, zeros) into the earliest andlatest tile-aligned candidate, or a refusal carrying both lengths. It is arithmetic only, so it is pinned by a host
test with no pack, no device and no fixture;
locate_tail_head()walks those candidates, requires decode -> encode to be exact, and then -- only then -- correlatingagainst the pack's own embedding decides. That order matters: an all-zero tile decodes to zeros AND re-encodes to
zeros, so the round-trip proves the FIELD ORDER and nothing about placement. Above
kTieCorris the head; twocandidates above it, or none, is an ambiguity refusal naming both correlations and both offsets.
Tied and untied are reported rather than assumed. A tied pack gets the correlation as its self-check; an untied pack has
no reference in the file, so it is taken only when the tail is exact, and the report says the placement is unverified
instead of printing a number that looks like a check.
tie_word_embeddingsis still read from the pack, never inferred.Gate 1,
Llama-3.1-8B_rai_1.9.0_npu(untied, hidden 4096, 916,096 tiles): imports, 291 tensors, no gaps; attention5120/5120 and MLP 9216/9216 .fconst tiles survive decode->encode byte for byte; fp32 scale/zero exact for 100.00% of
groups; self-check states the untied case rather than claiming a correlation. Regression,
Qwen2.5-0.5B-Instruct 1.9(tied, hidden 896): unchanged -- 291 tensors / 67,088 tiles, corr 0.999999, rel_err 0.000700, row-shifted control 0.0716.
Block-32 heads (the 1.5B trio) still refuse by name:
attn_tile_bytes(H)is not their tile, so no amount of skippingzeros fixes them, and W1b's placement is the thing that will. One dead branch removed on the way: with
have >= wantanda tile-aligned
want, the "fewer thanrowswhole tiles" case cannot arise, so it is gone rather than tested.