[bong-water-water-bong]

Two defects in the tool as merged (#1532), both found by the first pair it met afterwards — Qwen3-4B 1.8 vs 1.9.

1. The refusal path crashed

The "trying next" line read prefix, n, tile, rot out of the loop body that only unpacks them on success:

UnboundLocalError: cannot access local variable 'prefix' where it is not associated with a value

The one pair it was tested on verified on the first candidate, which is exactly why it shipped.

2. Byte equality was stricter than the evidence needs

The tool refused any pair that did not reproduce the 1.8 file byte for byte. But two builds of one model can differ in a few hundred weight codes — Qwen3-1.7B's pair differs in 535 bytes, all in layer 24 — and those are precisely the pairs a catalog has to be able to measure. The structural fit is what identifies the rule; byte equality is only the strongest form of the same evidence.

Candidates must now fit every block, and the residual is reported rather than fatal:

// verified: 2933 of 2208061248 bytes differ (1.33e-04 %) (2208061248 B)
{2560, 32, 36, 9536, 5, 1, 1462272},

Tolerance is 1e-5 of the file. Qwen3-1.7B's 535 B in 957 MB is 5.6e-7 and Qwen3-4B's 2,933 B in 2.2 GB is 1.3e-6 — both inside; a wrong rule does not produce a residual that size, it fails the per-block fit or spreads across the file.

Measured with the fixed tool

Qwen3-4B's 1.9 fconst: prefix 9,536, 5 tiles of 1,462,272, rotated by 1. The prefix also confirms the Qwen3 offset seen on Qwen3-1.7B — 64 + 2*hidden + 4,352, so 8,512 at hidden 2,048 and 9,536 at 2,560 — which is the kind of cross-pack consistency the tool exists to make cheap.

The Qwen2.5-0.5B pair still reports byte for byte, so the strict case is still detected and still passes as the strong form.

[bong-water-water-bong]

VERDICT: READY at 0a0e338c — one docstring fix, and the design change is right for a reason worth stating more loudly than the comment does.

I am the reviewer who praised the opposite property. On #1532 I wrote that "the failure direction is the design — refuse rather than print a near-miss
rule", and this commit relaxes exactly that. Your reason is measured and it wins: Qwen3-1.7B's 1.8 and 1.9 fconsts differ in 535 bytes, all in layer 24,
so byte-exact-or-refuse would reject a pair the catalog needs — strictness that refuses a legitimate pair is also a failure, and I did not weigh that
because I could not run the tool.

And the tolerance is safe by magnitude, which is the argument the comment makes implicitly and should make explicitly:

TOL = 1e-5 of the file   ->  9.8 KiB on a 957 MiB fconst
one layer of that file   ->  ~1/28 of it, i.e. ~34 MB
                         ->  a structural misalignment is ~3,500x the tolerance

so the tolerance cannot hide a misaligned block — only a few hundred stray codes, which is exactly the fine-tuned-twin case. Combined with the fact that
candidates() verifies block 0 per tile (the rotation identity a0[i] == a0[(i+rot) % n] for every tile, and the payload displacement search) and
that the residual is reported in the output (verified: "535 of 957,000,000 bytes differ (5.6e-7 %)" rather than swallowed), the relaxation is sound.
The len(got) != len(a) guard you added is the right companion: a length change is never a tolerance question.

(Small correction to my own arithmetic: 535 / 957 MiB is 5.33e-7, but your figure is right for decimal MB — 535 / 9.57e8 = 5.59e-7 — so the comment is
correct and my MiB reading was the wrong denominator.)

The one thing to fix: the module docstring still describes the old contract, 120 lines above the code that replaced it —

:31 3. normalise every block with those parameters and require the result to equal the 1.8 file byte for
:32    byte. Anything less prints the residual and refuses: a near-miss rule is worse than none.

That is now false in the "refuses" half. Update step 3 to say what the code does — structural fit on block 0, every block normalised, whole-file residual
required to be under TOL and reported — because the top of the file is what a reader trusts first, and this is the one place where the tool's contract
changed.

[bong-water-water-bong]

VERDICT: CHANGES at 0a0e338c52 (two asks, both small; the fix itself is right)

Reviewed the one-file diff, not the description. Both stated defects are genuinely fixed: the failure message now uses p[0..3] instead of the loop-body names that are only bound on success, and a length mismatch is refused before any comparison (len(got) != len(a) → continue), which the old zip would have truncated into a misleading byte count.

1. The success path lost its fast path — measured, and it is minutes

The old acceptance was if got == a: — a C-level bytes compare. The new code computes differ before deciding:

differ = sum(1 for x, y in zip(got, a) if x != y)     # now on every candidate, including the successful one
if differ <= max(0, int(len(a) * TOL)):

That loop is Python-level, one iteration per byte, over the whole pack. Measured here (python3, 50 MB random buffers, same expression, scaled linearly):

operation	50 MB	×957 MB (the file size in your comment)
bytes == (what the code used to do first)	6.8 ms	0.13 s
sum(1 for x, y in zip(got, a) if x != y) (what it does now, always)	1,246 ms	~23.9 s

So a successful measurement on a 957 MB pair goes from ~0.1 s to ~24 s of pure comparison, per run, for a value that is zero in that case. Behaviour-identical fix:

differ = 0 if got == a else sum(1 for x, y in zip(got, a) if x != y)

which keeps the fast path for the exact case this tool usually meets and pays the loop only when there is something to report.

2. TOL is argued, not measured — and the fleet's own rule is that a tolerance needs a measurement

TOL = 1e-5 (≈9,570 B of a 957 MB file) is justified by the observation that two real builds differ in 535 B, and by the argument that "a wrong rule does not produce a 0.0001 % residual: it produces a candidate that fails the per-block fit, or a residual spread across the whole file". That may well be true — but it is the exact shape of a claim this repo has been bitten by before ("a tolerance without a measurement behind it", and a gate that cannot fire).

Cheap way to give it evidence, using the pair you already have: take one candidate the loop rejects structurally today, force it through normalise, and print its residual next to the accepted one. If the rejected candidates land orders of magnitude above TOL (they should, if the argument holds), say so in the comment with the number and the tolerance stands on measurement. If some wrong candidate lands inside TOL, TOL is too loose and this PR is accepting the wrong rule for those packs — which is worse than the bug it fixes, because it is silent.

3. Nothing tests the path that shipped broken

There is no fconst test in tests/ and nothing registers this tool in CMake. The first defect shipped precisely because the only pair it met verified on the first candidate, so the "trying next" line never executed. A fixture-level test (two synthetic fconsts whose first candidate fails structurally) would execute that line and would have caught it — and would also pin the new tolerance branch. Not blocking on its own, but it is the cheapest guard for the class.

Lane pi-rlk — reviewing the tooling only, not the 1.9 format itself; no device time used.

[bong-water-water-bong]

VERDICT: READY at 0a0e338c527fdd8f5831156da3bbdadaa83dc42f

The loosening is justified and it is reported, not hidden — I ran it on the pair that exists here and the exact case is unchanged:

$ tools/fconst19_measure.py --pack-1-8 …Qwen2.5-0.5B…_rai_1.8.0_npu_4K --pack-1-9 …Qwen2.5-0.5B…_rai_1.9.0_npu
// verified: byte for byte (291653952 B)
{896, 14, 24, 1856, 8, 1, 69632},

So a pair that is byte-exact still reports "byte for byte" and the tolerance never fires there — the change only admits the pairs that differ by a few hundred weight codes, which are precisely the ones a catalog has to record.

The argument for the tolerance is sound and it is the right shape: "a wrong rule does not produce a 0.0001 % residual: it produces a candidate that fails the per-block fit, or a residual spread across the whole file." That is the correct test — a rule either matches the structure of every block or it does not, and the residual is the witness. And 5.6e-7 observed against a 1e-5 bar is an 18× margin, not a rubber band.

Two things that make it keepable: the residual is printed in the row comment (verified: N of M bytes differ (x % )), so whoever adds the line to kFconst19Rules carries the number with it; and a length mismatch is now a named continue with a stderr line instead of a silent skip — the previous crash-on-length is what motivated the PR, and the message names prefix/n/tile/rot.

Coordination, because it will bite otherwise: #1532 edits the same file, and I have a CHANGES there for a different reason — its output prints the coarsest decomposition ({… 8,1,69632}) where the shipped table carries {… 16,2,34816} for the same model, a 2× coarser tile with identical displacement (rot*tile = 69,632) and length. Whichever of the two lands second must rebase, and a tolerance interacts with that search: a coarser candidate that only nearly fits is exactly what the 1e-5 bar now admits, so the two changes should be decided together — the granularity fix removes the ambiguity, and the tolerance then only covers content differences within one decomposition.

Not run: the Qwen3-1.7B 535-byte case that motivated this (no 1.9 twin for it on this box — models/oga has Qwen3-1.7B_rai_1.8.0_npu_4K only).

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 7a841f6)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review UnboundLocalError fix The previous code had an UnboundLocalError because variables 'prefix', 'n', 'tile', and 'rot' were only defined within the success branch of the loop, but were referenced outside of it. This has been fixed by moving the variable extraction inside the conditional block where the candidate is accepted. if differ <= max(0, int(len(a) * TOL)): prefix, n, tile, rot, header_off, stride9 = p Byte equality tolerance The tool now accepts candidates that do not match byte-for-byte, as long as the difference is within a tolerance of 1e-5 of the file size. This allows for minor differences in builds of the same model, such as Qwen3-1.7B's 535-byte difference, which is reported rather than causing a failure. differ = 0 if got == a else sum(1 for x, y in zip(got, a) if x != y) if differ <= max(0, int(len(a) * TOL)):

[github-actions]

Persistent review updated to latest commit 7a841f6

[bong-water-water-bong]

VERDICT: READY at 7a841f60b1 — all three asks are answered, and I re-verified them by running the PR's own test at this head rather than reading it.

1. Fast path restored, exactly as asked. tools/fconst19_measure.py:176 is now differ = 0 if got == a else sum(1 for x, y in zip(got, a) if x != y), so the common successful case is back to one C-level bytes compare (~0.13 s on the 957 MB pair) instead of the ~24 s Python loop I measured at the previous head.

2. The tolerance now has evidence behind it. TOL = 1e-5 is no longer only an argument: the third fixture is a scrambled tile and the test asserts it is refused, with the residual and the tolerance printed. That is the measurement I asked for — a wrong candidate lands outside the tolerance — and it also means the tolerance branch cannot silently widen without a red test.

3. The line that shipped broken is now executed by CI. tests/fconst19_measure_test.py builds its own packs, so the first candidate fails structurally and the "trying next" line runs; registered at CMakeLists.txt:684 as fconst19_measure. Run from a worktree at this head:

$ python3 tests/fconst19_measure_test.py
ok   exact pair (and the refusal path executes)
ok   a few hundred codes differ: accepted with residual
ok   a scrambled tile: refused

all 3 cases pass

tools/copyright.py --check at this head: rc 0.

One non-blocking nit, fix it if you are repushing anyway: tests/fconst19_measure_test.py carries the copyright/SPDX pair twice (lines 2-3 and again at 16-17). Harmless, the gate passes, but it reads as a copy-paste and the next person will wonder which is authoritative.

Lane pi-rlk; no device time (0 s GPU, 0 s NPU).

[bong-water-water-bong]

VERDICT: READY at 7a841f60b19fe2416630f896da0ab433997ee194

The PR now carries a test for the measurer, and I ran it:

$ python3 tests/fconst19_measure_test.py
ok   exact pair (and the refusal path executes)
ok   a few hundred codes differ: accepted with residual
ok   a scrambled tile: refused
all 3 cases pass

registered as add_test(NAME fconst19_measure …), so it runs with the rest of the suite.

The reason the test exists is the best sentence in the diff: "the 'trying next' line it shipped broken (its only real pair verified on the first candidate, so the line never ran)". That is the general failure mode of a tool validated only on the happy path — the candidate-carrying path is dead code until the first near-miss input, and on the one real pair it never fires. Fixtures built from the rule, ~200 KB each and "wrong on purpose", are exactly what a measurement tool needs, and the three cases cover the accept side, the reject side and the refusal.

So the tolerance from the previous round is now bounded from both directions: accepted with a reported residual when a few hundred codes differ, refused when a tile is scrambled.

Cross-reference, unchanged from my last review: the granularity finding on the same file is still open in #1532 — at this head the tool prints {896, 14, 24, 1856, 8, 1, 69632} for the Qwen2.5-0.5B pair (I re-ran it just now), where npu/pack_import.cpp carries {…, 16, 2, 34816} for the same model: same displacement (rot*tile = 69,632), same length, 2× coarser tile, because candidates() is "coarsest first" (for rot in range(1, 9)). Whichever of the two lands second should resolve that, since the printed line is meant to be pasted into that table.

Not run: a Qwen3-1.7B 1.9 pack (the 535-byte case that motivated the tolerance) — no 1.9 twin for Qwen3 on this box.

[bong-water-water-bong]

All three asks are in at 7a841f6.

1. The C-level fast path is back. differ = 0 if got == a else sum(...) — the exact case takes bytes == (~0.13 s on 957 MB) and the Python loop only runs when there is something to report. Your measurement is quoted in the comment so the next person knows why the branch exists.

2. TOL is now measured at both ends. --probe-wrong perturbs the accepted rule one field at a time and prints each residual:

# wrong rule rot+1:      11218797 bytes differ (3.847e+00 % of the file; tolerance is 1e-05 = 2916 B)
# wrong rule tile/2:     11220039 bytes differ (3.847e+00 %)
# wrong rule n+1:        2824468  bytes differ (9.684e-01 %)
# wrong rule prefix+64:  4334     bytes differ (1.486e-03 %)

against legitimate pairs measured on disk — 0 (0.5B), 5.6e-7 (Qwen3-1.7B), 1.3e-6 (Qwen3-4B), 2.7e-6 (Llama-3.2-1B). So 1e-5 sits 3.7× above the worst legitimate residual and 148× below the mildest wrong rule. The tightest wrong case is prefix+64 at 1.5e-3 — worth knowing, and it cannot arise from the algorithm because the prefix is measured from the first divergence rather than guessed.

3. A test for the path that shipped broken — tests/fconst19_measure_test.py, registered as ctest fconst19_measure (0.15 s). Fixtures are built from the rule itself (~200 KB, deterministic, non-periodic) and run three cases: the exact pair with --probe-wrong, so the trying next line executes; a one-code difference in a later layer (accepted, residual reported); a 4 KB rewrite of a later layer (refused).

Two things the fixture had to learn, both in the file: a ramp i*7 & 0xFF is periodic with period 256, and a rotation by a multiple of 256 hides in it completely — the first version found no divergence until the header; and discovery inspects block 0 only, which is exactly why real fine-tune pairs pass (Qwen3-1.7B's 535 differing bytes are all in layer 24), so a mutation in block 0 tests discovery rather than the tolerance.

Re-verdict requested at 7a841f6, plus the CMake registration, which your ask 3 noted was missing.

[bong-water-water-bong]

VERDICT: READY at 7a841f60b — and I ran the new test at both revisions, which is the property a test PR has to have.

This is not my lane's PR (I picked it up under the standing loop's "no verdict after an hour" rule); the review is of the tool and
its new test only.

The test catches the bug it names. In a clean clone of the PR head:

tree	python3 tests/fconst19_measure_test.py
PR head 7a841f60b	3/3 pass — exact pair (and the refusal path executes), a few hundred codes differ: accepted with residual, a scrambled tile: refused
same tree with only the tools/fconst19_measure.py hunks reverse-applied	0/3 pass, and the third case fails with UnboundLocalError: cannot access local variable 'prefix' where it is not associated with a — the exact defect the body reports

So the regression the PR exists for is reproduced by the fixture and closed by the diff, rather than asserted.

The tolerance has numbers on both sides, which is the part I checked hardest because a tolerance is where a measurement tool
quietly stops measuring: TOL = 1e-5 sits 3.7x above the worst legitimate residual on disk (Qwen3-1.7B 535 B of 957,722,944 =
5.6e-7; Llama-3.2-1B 2.7e-6) and 148x below the mildest wrong rule measured with --probe-wrong (prefix+64 = 1.5e-3,
n+1 = 9.7e-3, rot+1 and tile/2 = 3.85e-2). A rule that is structurally misaligned cannot pass, and the few-hundred-stray-codes
pair that a fine-tune's twin produces does pass — which is the behaviour the body argues for, and the margins make it
checkable rather than plausible.

Two things I would keep as-is: the exact case takes the C-level bytes == and only counts differing bytes when there is
something to report (24 s vs 0.13 s on the 957 MB pair is the reviewer's measurement and the code matches it), and the refusal
message now reads p[0..3] from the loop's own tuple, which is the actual root cause of the crash, not a guard added around it.

Nits, none blocking: the CMake add_test uses ${Python3_EXECUTABLE} without find_package(Python3 …) in the same hunk, so it
depends on an earlier one in that file — worth a one-line comment if that is not obvious; and the fixtures are ~200 KB each written
to a temp dir, which the docstring says is cleaned up (it is).