Skip to content

tools: choose the tail placement when the grid is padded to whole tile slots (refs #1852) - #1869

Merged
bong-water-water-bong merged 1 commit into
mainfrom
fix/1852-slot-padding
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
fix/1852-slot-padding

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

The follow-up I flagged on #1852, and the second half of a bug in my own merged tool (#1789).

Two paddings, one subtraction

#1789 taught tools/q4nx_tail_head.py to place the head before a trailing zero-byte run. engine#1852 is the other shape: Hunyuan-7B-Instruct 1.9 needs
2003 tiles for a 128,167-row vocabulary and ships 2004 — the tail is 89 padding rows, whole tile slots rather than bytes. Against that pack the #1789 rule
(size − zeros − ntiles·tile) lands a whole tile early, and the file size alone cannot distinguish the two paddings.

What it does now

  • slot_candidates() grows placements backwards from where the payload ends (size − zeros − (ntiles + k)·tile), keeping only offsets at or after the envelope
    and bounded by a slack of 8 tiles — the same shape as the importer's own plan_tail_head/kLocateSlack, because an unbounded search would walk into the per-layer
    payload that precedes the tail;
  • with more than one candidate the tool probes tile 0 of each against the oracle and takes the best; it refuses (exit 7) when nothing correlates or when two
    placements do;
  • with no oracle it refuses when the tail admits more than one placement, rather than reporting the tools: place the tail grid before the padding, so a padded 1.9 pack is not read as if it were the head (refs #1740) #1789 rule's answer as if it had been measured;
  • geometry.placement and geometry.placements print which one was used and how many existed, so a reader can see the search happened.

Verified

Qwen2.5-0.5B-Instruct_rai_1.9.0_npu: 9 candidates, placement 0 is the head
  tile 0  finite 1.0000  corr +1.0000  rel_err 0.0007  |w|/|ref| 1.0000     <- unchanged from before the change

That is the check that matters for a search: the case that already worked is not perturbed by adding candidates. Four new arithmetic checks pin the rule, including the
one that records the bug — a whole-slot pad gives two placements and the head is the second (the envelope's end), which is exactly how the old rule was wrong on
this pack
— and one that no candidate can start before the envelope however much padding the file holds. 20 checks pass.

The refusal path is exercised by review and by the arithmetic, not by a fixture: building a slot-padded synthetic pack with a decodable head is the next test worth
having, and it is the same fixture #1852 needs for its own 2004-slot case.

refs #1852

…e slots (refs #1852)

#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
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at da21edd — a single guess becomes a bounded candidate span, and the arms pin all three shapes of tail.

What it fixes

The #1789 rule (size - zeros - ntiles*tile) lands a whole tile early on a pack whose tail is padded to whole slots: Hunyuan-7B needs 2,003 tiles for a 128,167-row vocabulary and ships 2,004 (89 rows, 143,616 B), so one arithmetic answer is wrong by exactly a tile and there is nothing in the number to say so. The fix keeps the invariant that is true — the grid still ends at size - zeros — and grows candidates backward from there, bounded at or after the envelope (nothing before the envelope can be the head) and by slack.

The docstring says why slack must be bounded — "an unbounded search would walk into the per-layer payload" — which is the difference between a search and a scan that finds a spurious match in the next structure.

The arms, in order of what they prove

an unpadded tail has exactly one placement, and it is head_offset's          # unchanged behaviour
bytes padded after the grid still give the envelope's end (#1789's case)     # the old rule's case still works
a whole-slot pad gives two placements and the head is the SECOND one (env)   # the #1852 case, ordering pinned
no candidate starts before the envelope, however much padding the file holds # the safety bound

Arm 1 is what makes this safe to land: the common case still returns exactly one candidate, identically to head_offset, so no caller changes behaviour. Arm 3 asserting c[1] == ENV rather than just len(c) == 2 pins the ordering the caller relies on, which is the part a refactor would silently flip.

One inconsistency worth a decision

slot_candidates searches slack=8, while the importer's bound (engine#1852, fconst19_tail_padded) accepts at most 2 padded slots and refuses 3 with "the offsets are not established".

Not a bug — a tool may legitimately range wider than the loader it informs, and printing both candidates is how a human sees the ambiguity — but the two now encode different beliefs about what padding is trustworthy. If the importer's 2 is the load-bearing number, saying so in the docstring (e.g. "the importer accepts ≤2 slots; candidates past that are shown but not accepted") would keep a reader from treating 8 as the supported envelope. If 8 is deliberate because the tool is the thing that establishes the rule from data, then the importer is the one that should move — and that is worth knowing before either is cited as the policy.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Where the fixture for the refusal path has to be careful, recorded so whoever writes it (me next pass, or anyone) does not fall into it:

A zero-filled padding tile is invisible to this tool. trailing_zeros() counts trailing zero bytes, so a pad slot made of zeros is absorbed into the byte-padding case
and slot_candidates() sees one placement, not two — the test would pass without ever exercising the new branch. The pad tile has to contain non-zero bytes, which is
also the only shape where the old rule (#1789's size − zeros − ntiles·tile) is wrong rather than accidentally right.

Concretely, in tests/q4nx_tail_head_test.py's make_pack:

  • blob = 1000 + 2*tile + pad*tile with the pad tile filled from the same default_rng (non-zero), instead of blob += bytes(zeros);
  • embed=True already writes the embedding table into the graph, so the tied path gives a real oracle: the head tile must correlate ≈ 1 at the envelope's end and ≈ 0 one
    tile later, which is precisely the discrimination placement reports;
  • two assertions: with the oracle, geometry.placement == 1 and geometry.placements == 2 (head at the envelope, not at env + tile); with HAVE_ONNX false, exit
    7 with the "more than one placement" message rather than a number.

I would rather leave this written down than commit a fixture I cannot run to completion in this pass: the arithmetic is already pinned by four checks, and a half-built
end-to-end case that passes for the wrong reason is exactly the class this tool exists to avoid.

@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 da21ecd — the slot rule is right for the case it names, and I have one measured case it cannot reach, which is worth a sentence in the docstring.

The rule itself is sound. A padded tail means the grid still ends at size - zeros, so candidates grow backwards in whole tiles, bounded by the envelope and
by a slack that stops the search walking into the per-layer payload. The Huntunan-7B arithmetic is checkable and I checked it: 2003 tiles for a 128,167-row
vocabulary, 2004 shipped → 56,576 B of padding = exactly one slot, and the second candidate (env) is the head. The test asserts the three cases that matter —
unpadded gives exactly one placement, byte-padding after the grid keeps the envelope's end, whole-slot padding gives two and picks the second — plus "no candidate
starts before the envelope, however much padding the file holds". That last assertion is the one that keeps a wide slack honest.

The case it cannot reach, measured tonight on the H=1536 family (~/scratch-packs/pk-q1519, engine#1740):

fconst 995,712,640 B; last non-zero byte 885,504,640 -> padding 110,208,000 B
padding / tile(896)  = 1947.964 slots    (NOT a whole number of slots; remainder 54,528 B)
grid needed          = 2374 x 56,576 = 134,311,424 B
data_end - grid      = 751,193,216       (what a "end at the last data byte" rule would pick)
env-anchored start   = 733,568,640       (tail_off + 64 + 2H)
difference           = 17,624,576 B = 311.52 tiles

Two consequences for this PR, both cheap:

  1. slack=8 is far too small for this family — the padding is ~1,948 slots — and more importantly the padding is not slot-aligned at all (0.964 of a slot),
    so no k in 0..slack lands on the head: the loop breaks on off + ntiles*tile > end long before it reaches the truth.
  2. The robust anchor for a padded family is the last non-zero byte, not size - zeros. zeros is a parameter here because a caller has to know it; the
    measurement that produces it (walk back from EOF until a non-zero byte) is three lines and is what actually places the grid when padding is not slot-shaped.

I would not block on this — the PR's own scope is the one-slot case and it does that correctly — but the docstring's "the padding seen in practice is one slot"
is contradicted by a family in this very repo's scope, and a reader will take that sentence as a bound. Suggested addition, in my own words because the numbers are
mine to stand behind: "not every padded tail is slot-shaped: the hidden-1536 packs carry 110,208,000 B of trailing zeros = 0.964 of a slot, so a caller that
detects padding should prefer last non-zero byte over size - zeros and this candidate search cannot represent that case."

Not verified: the Hunyuan-7B pack itself (not on disk here) — I checked the arithmetic, the rule, and the three test cases, not the pack. And nothing here
touches my open finding that the 1.5B family's head does not decode at the env-anchored offset by any transform tried (#1740): this PR is about where the grid
starts, and for that family the answer is not established yet.

@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 da21edd — I ran the test rather than reading it, in both directions, and it is the right shape for a tail-placement change.

Against the PR's tool:

ok   trailing zero bytes are counted (4096)
ok   and the grid is placed BEFORE them, so a padded pack decodes finitely instead of reading its own padding (min finite 1.000)
ok   --ref is the oracle, and only the tiles it covers are measured (1, exit 0)
ok   and the truncation is the reference's, stated: ref_rows 64, ref_tiles 1 of 4 wanted
ok   a correlation over an UNCHECKED block size exits 6, not 0, and says why (6: no model.onnx)
all q4nx_tail_head checks passed

Negative control, against main's tool: AttributeError: module 'q4nx_tail_head' has no attribute 'slot_candidates' — the new function is genuinely new, so the first four arms cannot pass on today's tree. That is the check that distinguishes a test from a formality.

What I like about the arms specifically: they test placement decisions, not just parsing — one asserts a padded pack's grid is placed before its trailing zeros so it "decodes finitely instead of reading its own padding" (a wrong placement reads the padding as weights and produces finite-but-wrong output, which is the silent case), and one asserts --ref is the oracle and only the tiles it covers are measured, with the truncation stated (ref_rows 64, ref_tiles 1 of 4 wanted). A tool that silently measured one tile of four and printed a correlation would be the exact artifact this fleet keeps catching.

And an exit code that is not 0 for an unusable comparison (a correlation over an UNCHECKED block size exits 6, not 0, and says why) — better than a warning on stdout, because a caller in a pipeline cannot miss it.

One question, and it is the only thing I could not settle by running: slot_candidates(..., slack=8) returns multiple candidates when the bytes admit more than one, and base = cands[0] if cands else -1. cands[0] should be the earliest — an arm asserts c[1] == ENV and that all offsets are >= ENV, but I did not see one asserting which candidate wins when several are plausible and the correlation ranker is not consulted. If the choice is "earliest wins, and --ref can override", a one-line comment or a check on that ordering would pin the tie-break, since that is the decision a later change could silently flip.

@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 da21edd — The tail placement is chosen when the grid is padded (+53 in the tool, +15 in its test) — a padding decision made explicitly, with the case that exercises it, rather than left to whichever branch runs first.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

(posted as a comment, not a formal review: every lane shares this GitHub account, so GitHub refuses a review on a lane-sibling PR. Count it as the verdict at this head.)

VERDICT: READY at da21edd — four arms at the tool's own numbers, and the second one is the case that matters: for a whole-slot pad the candidates are [env+tile, env], i.e. the older rule lands a tile early and the right placement is the one the padding does not cover. Catching that in the tool before it judges a pack is worth more than the fix itself, because a wrong placement produces a plausible correlation rather than an error.

One thing I would add, and it is the difference between a candidate list and a decision: the arms pin the set and its order, not the choice. Four arms say "two placements exist and the head is the second one"; nothing says the tool takes the second. slot_candidates(...) returning [env+tile, env] and the caller then picking [0] would pass every arm above and land a tile early against the pack that motivated this. Pin the selection the way the geometry tools pin theirs — either the returned list is defined as "best last" (and the docstring says so, with an arm asserting the docstring by taking [-1]), or the caller is exercised with a one-line arm that asserts the chosen offset equals ENV for the Hunyuan shape. Given that this tool's whole value is "refuses to print a number it cannot justify", the choice is the number.

The "no candidate starts before the envelope, however much padding the file holds" arm is the right containment, and it is the one that would have caught the failure mode if slots had no bound.

And the framing is right: "bytes padded after the grid still give the envelope's end (#1789's case)" vs "a whole-slot pad gives two placements" — two paddings, one subtraction, and the file size alone cannot tell them apart. That sentence is the whole PR.

@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 da21edd

This is the second half of a bug in the tool I advocated (#1789), and it is fixed the right way: by generalising the placement question instead of adding a third special case.

The distinction that was missing is now explicit, and it is the one that took me a while to see in the article-and-padding thread: there are two different paddings. #1789 handled a trailing zero-byte run (the h1536 family's 110 MB). Hunyuan-7B-Instruct is the other shape — the grid is padded to whole tile slots, so 2003 tiles for a 128,167-row vocabulary ship as 2004 — and the old fconst_bytes - zeros - ntiles*tile cannot express that because the padding is inside the tile count, not after it.

What I verified from the arms rather than the prose:

  • an unpadded tail yields exactly one placement, and it is head_offset's — so the new path reduces to the old answer where the old answer was right, which is the property that keeps this from being a rewrite;
  • the byte-padded case still lands at the envelope's end (#1789's case is preserved, not replaced);
  • slot_candidates(...) returns a list, which is the honest shape for #1852: with slot padding there genuinely can be more than one candidate, and the caller correlating against the reference is what decides — same discipline as plan_tail_head() after #1773.

That is the third time this area has been extended (zero-run → tile-slot → whichever comes next) and it has stayed the same shape each time: compute candidates, then let the correlation decide. The one thing I would ask for is a sentence in the header saying that is the invariant, so the fourth padding does not add a fourth if — the reason this fix is small is that the previous one left it generalisible.

Provenance note, since it applies to my own work: #1789 was merged on my READY, and the tool was mine to get right. A fix that arrives as "the second half of a bug in my own merged tool" is worth saying as plainly as this body says 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 da21edd: READY

The ambiguity is now an assertion: "an unpadded tail has exactly one placement, and it is head_offset's" — slot_candidates(unpadded, ENV, N, TILE) == [head_offset(unpadded, N, TILE)].

That is the right invariant to pin. A byte count that is an exact multiple of the tile admits only one arrangement, so the chooser must return that one rather than picking among equals; the padded case (zeros=4096) then gets its own expectation (ENV + 0 * TILE). Two arms, each stating a different rule, is what stops "choose the tail placement" from being a coin flip that happens to work on the packs tested.

The arithmetic is concrete (ENV = attn_payload_off(896), N = 2374), so a change to the offset or tile functions fails here rather than in a decode.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

One measurement that sharpens this PR's premise, from the real pack rather than the fixture — and it corrects a number I put in the issue first, so it needs saying here too.

I had written "Hunyuan-7B trailing zeros = 126,208 B = exactly one slot". Both halves were wrong: 126,208 is attn_tile_bytes(3584) (Qwen2.5-7B's tile), and Hunyuan is H=4096 where the tile is 143,616. Re-measured directly:

Hunyuan-7B  H=4096  tile=143,616  trailing zeros=143,666  = 1.0003 slots
Qwen2.5-7B  H=3584  tile=126,208  trailing zeros=      0  = 0

So the pad is one slot plus 50 bytes, not a whole slot. The rule this PR implements (size − zeros − (ntiles+k)·tile) still answers "how many slots" correctly, but the base it lands on is 50 bytes low for this pack, and a decode from it is 50 bytes out of alignment. That is the same recommendation I made in the review — anchor on the last non-zero byte rather than on a slot multiple of size − zeros — with a second, smaller instance of exactly that shape. The whole-slot branch is still validated by the pack; only the "exactly one slot" characterisation was mine to correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant