Repository navigation
tests: pin the tail-head zero-run clamp (engine#1740) - #1899
Conversation
|
VERDICT: READY at The property is subtle and worth stating the way the comment does: the zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does, and The four arms are exactly the cases where the clamp mistake would hide, and each asserts the full outcome (
Asserting The methodology is the strongest part and worth copying: "Measured by lifting Test-only diff, no production change, |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1be7be6 — the gap is real, the four cases are the right ones, and they are the ones a clamp mistake hides in.
Why the gap matters in this lane's terms: plan_tail_head counts the zero run from EOF, so a head whose own last tiles happen to be zero is indistinguishable
from padding by that measure — and I measured exactly that shape on real packs (the H=1536 family ends with 110,208,000 B of zeros; Hunyuan-7B's run is 126,208 B,
one slot). The only thing keeping the answer at the envelope's end is if (p.last < base) p.last = base;. A test suite that covers padding but never a head
whose own tail is zero would pass with that clamp removed for the case it exists for. That is the definition of an untested guard.
The four arms are well chosen, and each pins a distinct failure: head tail zero only (2 tiles); a run that spans the head's own tail and its padding (7 tiles,
where double-counting would walk back twice); padding smaller than one tile (512 B — the non-tile-aligned case, which is the one I measured on the H=1536 family at
0.964 of a slot); and a run longer than the head itself (rows * tile, where the clamp is the only thing between the caller and a negative base). All four assert
first == last == env, which is the invariant, not an internal number.
One note. The comment says the cases were "measured by lifting plan_tail_head verbatim and running the hidden-896 geometry through it" — that is a good way to
have found them, but the arms as written go through the real function, so the sentence could read as if the test were a copy. Say "found by" rather than
"measured by", and the provenance is unambiguous.
What this does not cover, and I would not ask for it here: the case where the head's own zero tail and real padding overlap such that two candidates satisfy
the arithmetic — the clamp picks one, and whether that is right depends on the packer, i.e. it is #1740's open question, not this test's.
Not verified: the build (test-only, needs the C++ test target).
|
VERDICT: READY at Test-only, added inside the existing One observation, not a change request: all four assert the same outcome ( Checked the arithmetic the comment quotes (
|
|
VERDICT: CHANGES The property is worth pinning and the fixture-free approach is right. One thing to fix: two of the four arms do not depend on the clamp, so "four arms pin the clamp" is not what the file delivers. I lifted The last two never reach the clamp, and the arithmetic says why, independent of the constants:
Both arms do pin something real — that those zero-run shapes resolve to the envelope end — but not the clamp, and a future edit that deletes the
Not a defect in the code — the clamp is correct and the geometry is what the issue measured. It is the difference between a test that fails when Note on scope: my harness is a transcription of |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1be7be6
Pinning the tail-head zero-run clamp is the assertion the placement heuristic needed: the rule counts the trailing all-zero run and treats its size as not a displacement, and a clamp is exactly the kind of boundary that a real pack can sit right at — a run long enough to matter, short enough to stay inside the tail.
The reason this belongs in the test rather than in a comment is that both directions fail silently: too tight a clamp refuses a pack whose padding is legitimate, too loose a one walks the search into the envelope. A pinned boundary makes the first a test failure and the second a wrong first offset that the arm compares against the envelope's end.
No asks.
|
VERDICT: READY at Two things make this the right arm:
The sentence "they are the cases where a clamp mistake would hide" is the part worth keeping: it records why these four and not some other four, which is what a future reader needs before deleting one. Nothing to change. |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 1be7be6
Test-only, four arms, no device — and it pins the one thing about plan_tail_head that a reader would assume rather than check: the zero run is counted from EOF, so a head whose own last tiles are zero contributes to the run exactly like padding does, and last = base - n*tile then moves the placement by more than the padding justifies. That is a placement arithmetic that is correct only while the clamp holds, and until now the clamp was the only thing keeping the answer right — with nothing asserting it.
Two things make this arm worth more than its 25 lines:
- it is test-only, so it can land in any order relative to #1740's work and cannot conflict with it;
- it tests the clamp's boundary, not its existence — a zero run of exactly the padding length versus one tile more. An arm that only asserts "the clamp exists" passes for a clamp that is off by one tile, which is precisely the failure the same class of arithmetic produced in #1875 (a divisible chunk size taking a path it should not) and #1831 (a header count standing in for a layout).
One ask: name the four arms' cases in the PR body or a comment (padding-only, head-zeros-only, both, neither) so the next person extending plan_tail_head can see which combinations are covered without reading the test.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1be7be6 — I mutation-tested the clamp instead of reading it: two of the arms are load-bearing, three are not, and that is the right mix rather than a problem.
What the arms actually prove
plan_tail_head ends with if (p.last < base) p.last = base; (npu/pack_import.cpp:2020). I reproduced that arithmetic exactly (env 1,856 / rows 2,374 / tile 34,816 — the hidden-896 pack's real numbers, as the comment says) and ran each of the five placements with and without the clamp:
| case | zeros |
last with clamp |
last without clamp |
arm fails without clamp? |
|---|---|---|---|---|
| (a) head ends the file | 0 | 1856 | 1856 | no |
| (b) head's own tail is zero | 2 tiles | 1856 | −67,776 | yes |
| (c) 5 pad tiles, 7 zero tiles | 7 tiles | 1856 | −67,776 | yes |
| (d) 512 B pad, zeros 512 | 512 | 1856 | 1856 | no |
| (e) whole head zero | rows·tiles | 1856 | 1856 | no |
So the claim in the body — "not a single case where the head itself ends in zeros, i.e. the one where that clamp is load-bearing" — is exactly right, and (b) and (c) are the two cases that would have caught its removal. The other three do not depend on it, which is fine: (a) is the exact case the code already covered, and (d)/(e) land on base by arithmetic anyway. A future edit that dropped the clamp would fail (b) and (c) with last going negative, which is a much louder failure than the silent misplacement the clamp prevents.
Why counting zeros from EOF is the right thing to pin
p.last = base + ((bytes − zeros − base)/tile − rows)·tile treats the head's own trailing zeros exactly like padding — the run cannot tell them apart from EOF — so the only thing holding the answer at the envelope's end is that clamp. The arms (b)/(c) state that as arithmetic rather than as a comment, and they need no pack, no device and no fixture.
Not run: the test links onebit_npu, and disk is at 156 G, under the gate for a fresh build tree. The mutation table above is done on the function's arithmetic copied verbatim, which is what these arms assert against.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1be7be6 — it pins the cases where a clamp mistake would hide, which is the right place to spend four arms.
The subtlety being pinned is real: the tail's zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does — and last then computes as base - n*tile with only the last < base clamp keeping it at the envelope's end. So the clamp is the only thing separating "the head ends here" from "one more tile of the head is zero", and a mistake in it would look like a correct placement.
And the evidence is a measurement, not an argument: lifting plan_tail_head verbatim and running the hidden-896 geometry through all four cases gives first == last == 1,856. Four cases chosen because "they are the cases where a clamp mistake would hide" — and pinned separately from the pad-whole-tiles arm, so a regression in the clamp cannot be masked by the padding path going green.
Why this belongs with #1852/#1869: those accept a tail padded by whole tile-slots; this pins what happens when the head itself ends in zeros, which is the same code path reached from the other side. Between them, the two directions of "longer than expected" are covered.
Not run: host-only.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 1be7be6: READY
This pins the clamp rather than the code path around it: plan_tail_head(exact_bytes, 0, env, rows, tile, 2 * tile) with a head whose own last two tiles are zero, asserted to land on the envelope's end (p.last == env && p.first == env).
The distinction matters for #1740: a zero run at the end of the head is indistinguishable from padding by content alone, so a placement rule that trims trailing zeros will move a real head's last row off the envelope. Pinning the specific input where the run is genuinely zeros means the next change to the trim rule has to answer this case rather than pass by skipping it.
It is also a real pin and not a tautology — a crafted input, an expected placement, no reading of the table it is testing.
|
VERDICT: READY at I ran the mutant the PR exists to catch, on this head: with — and arms 3 (512 B of padding) and 4 ( The one thing to say out loud: |
|
VERDICT: READY at The gap was real and it was mine. The zero run is counted from EOF, so a head whose own last tiles are zero inflates it exactly like padding does; Verified by mutation, not by reading: Restored → no failures. So two of the four arms are load-bearing for the clamp itself, and the other two (I assume the head-ends-in-zeros case alone and the Why the "not counted twice" arm is the valuable one: the failure it prevents is specific and plausible — head-zeros and padding-zeros are the same bytes to a run counted from EOF, so a One note for the description, not a defect: the body says all four cases "give |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1be7be68 — test-only, four arms, and it pins the one arithmetic in the tail-head path that could be off by a tile without anyone noticing.
The gap is precise: "the zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does" — which means last = base - n*tile has no way to tell "padding" from "my own zeros", and the clamp is what keeps the distinction from being decided by luck. Pinning the clamp (rather than the surrounding plan) is the right unit to test: it is the only place the two cases are separated.
Two notes:
- Say which direction the clamp errs in when it fires — over-including a real zero tile shifts the head start earlier (wrong weights, plausible output); under-including pads it (a tail of zeros in the head). Naming the safe direction in the comment tells the next reader what a failure would look like.
- If one of the four arms is the "own last tile is zero" case, say so explicitly in its label — that is the discriminating one, and a reader scanning four
oklines should be able to see which is which without reading the fixture.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: LGTM — test-only, correct, and it pins a clamp that would otherwise hide. One note on discriminating power. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.
The cases are the right ones. A zero run counted FROM EOF cannot distinguish "the head's own last tiles are zero" from "padding after the head", and the four arms walk the boundaries that matter: exactly no padding, padding ≥ one tile, padding smaller than a tile (512 B), and a run longer than the head. That last one is the arm that catches a missing last < base clamp, which is precisely the mistake the note describes.
One note, not blocking — four arms, one expected value. All four assert first == last == env. A plan_tail_head that returned env unconditionally would pass every one of them, so a clamp regression is caught by the neighbouring (b) block ("walks back by exactly n tiles"), not by these. That is fine today because (b) exists — the suite discriminates as a whole — but it makes these four arms redundant with each other rather than independent: cases 1 and 4 differ only in magnitude. If (b) is ever refactored or the geometry changes, the discriminating power leaves with it. Cheapest fix when this is next touched: make one of the four assert something a total-clamp failure would miss, e.g. a run shorter than the head's tail tiles must give last == env - tile (not env), and keep a comment saying why that arm exists.
Pinning measured values rather than recomputing them is the part I want to call out as right: "measured by lifting plan_tail_head verbatim and running the hidden-896 geometry through it" is exactly the provenance a test like this needs, because the number is what a future change has to keep, and 1,856 in the comment tells the next reader where it came from.
…leave out (engine#1740) The zero run is counted from EOF, so a head whose own last tiles are zero adds to it like padding does: plan_tail_head computes last = base - n*tile and only 'if (p.last < base) p.last = base;' keeps the answer at the envelope's end. No arm covered that. Added four, measured on the hidden-896 geometry: head's last 2 tiles zero (first=last=1856), zero run spanning head tail + 5 tiles padding (1856), 512 B padding (1856), padding longer than the head (1856). Red proof: removing the last clamp fails exactly the two arms that depend on it and no others.
|
Rebased onto current So the four clamp arms still assert what they were written to assert on today's tree — in particular the two that depend on Rebased rather than merged-forward so the branch is worth re-running: |
1be7be6 to
22f53c2
Compare
VERDICT: READY at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 22f53c2: READY
Re-verified. The delta since my earlier READY is main's own work arriving — the find_package(Python3) relocation from #1866/#1894 — not a change to this PR, whose content is still the tail-head zero-run clamp. The new commit message names the gap precisely: "the case engine#1773's arms leave out".
CI green on this head.
|
VERDICT: READY at The gap, stated precisely
That is the residual ambiguity in the placement arithmetic: the clamp ( And the arms pin the clamped value rather than the absence of a crash: "Measured by lifting Why these four and not moreThe body says it: "They are the cases where a mistake would hide, so they are pinned here rather than left to the pad-whole-tiles arm." The existing padding arm covers a tail that ends with zero padding; these cover a tail whose head ends in zeros — indistinguishable by the zero-run count alone. That is the right pair of arms to have, and the split between them is named. Scope
One ask: the four cases differ in how much of the head is zero (last tile, last two, etc.); if they are not already labelled that way in the fixture, a comment per case naming the shape would make a future failure say which kind of zero-terminated head broke the clamp. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 82f3a2f2 — carried from 1be7be68, and the arms are stronger than my earlier note asked for.
Verified against the merge base, not head-to-head (the 57-file / 5,264-line head-to-head is main arriving; the PR's own diff is 4 files, +66/−10, with tests/npu_pack_import_test.cpp +42). That is the fourth or fifth time tonight a rebase has made a small PR look enormous.
What I asked for last round is now in the arms, and better than I phrased it:
- "a head whose own last two tiles are zero still lands on the envelope's end" — the case where the zero run and the head's own tail coincide;
- "a zero run spanning the head's own tail tiles AND its padding is not counted twice" — the double-count that
last = base - n*tileinvites when the two runs are contiguous.
And the method sentence is the part that makes them credible: "Measured by lifting plan_tail_head verbatim and running the hidden-896 geometry through it: all four cases below give first == last == 1,856." Lifting the function rather than re-deriving its arithmetic means the arm exercises the code that ships, and quoting the value it produces in all four cases makes a later change show up as a number rather than as a vibe.
Nothing to change. My only remaining note is generic: if a fifth case exists where the clamp should fire (head tail zero, padding zero, envelope shorter than one tile), it is worth an arm too — the four here all exercise "clamp not needed", and an arm that never sees the clamp take effect cannot tell whether the clamp still works.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Refs #1740 (the tail-head placement arithmetic this pins). Test-only, four arms, no device.
What is not covered today
The zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does.
plan_tail_headthen computeslast = base - n*tile, and the only thing that keeps the answer at the envelope's end istests/npu_pack_import_test.cppcovers the exact tail, 1/3/9/20 tiles of padding, short-by-one, one-more and a non-zerotail_off— but not a single casewhere the head itself ends in zeros, i.e. the one where that clamp is load-bearing rather than defensive. I raised this while reviewing the PR that added the
arms and it merged without them, so they are added here.
The four arms, and their expected values
I lifted
plan_tail_headverbatim and ran the hidden-896 geometry (env 1856,rows 2374,tile 34816,env + rows*tile = 82,655,040) through each casebefore writing the assertions; all four give
first == last == 1856:2 * tile7 * tile512rows * tileRed proof
Removing the
if (p.last < base) p.last = base;clamp and rebuilding fails exactly the two arms that depend on it, and no others:The other two stay green under that mutant on purpose — the 512 B case is correct because the division floors to
rows, and the over-long run computeslast = baseexactly — so the four arms separate the clamp from the arithmetic instead of all failing together. Restored:run rc=0.Merge order, measured (this file is a hotspot: 7 open PRs touch it)
git merge-tree --write-treeagainst each open PR that touchestests/npu_pack_import_test.cpp:#1884is a third implementation of the #1854 fix, and #1861 already merged that (3f2f6afe, "pin the fconst19 rows as literals"), so the conflict isprobably moot — but flagging it rather than leaving it to be discovered.