Skip to content

npu-pack: a tied head must correlate AND not be mis-scaled, or it imports cleanly and answers nonsense (engine#1503) - #2101

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/1503-rel-err-gate
Nov 11, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/1503-rel-err-gate

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes the class behind two packs that imported with every check green and served garbage — the import already printed the statistic that told them apart, as an advisory field.

The measurement

Three packs, same pipeline (engine abd2cb1e0, --weight-layout b8, my kernels-1536x2048-b8 / kernels-2304x4096-b8), one npu-hold ticket each for the serves:

pack corr rel_err serves
amd/Qwen-2.5_1.5B_Instruct_rai_1.9.0_npu 0.996901 0.000000 "The capital of France is Paris." ×3 identical
amd/gemma-2-2b-onnx-ryzenai-hybrid 0.991539 0.136140 myſelf myſelf … ×3 identical
amd/gemma-2-2b-awq-uint4-asym-g128-lmhead-g32-fp16-onnx-hybrid 0.996053 0.094874 myſelfGetBytes… ×3 identical

A head that is a scaled copy of its own embedding still correlates, so the correlation-only check passes it — and the import printed rel_err next to the correlation it gated on. Both packs cost a device ticket to find out.

The change

  • one predicate, selfcheck_head_ok(corr, rel), so the four sites that set selfcheck_tied cannot drift apart (they are the four decode paths: MatMulNBits, external-data, container and tail);
  • kSelfcheckRelMax = 0.02 — the measured gap: two orders of magnitude above the pack that serves, ~5× below both that do not;
  • Report::selfcheck_scaled, set at all four sites beside selfcheck_tied;
  • main() refuses on it and names both numbers and the threshold. --allow-incomplete deliberately does not override: that flag means "write the tensors I could not represent", and this is "the tensors I wrote do not decode".

Verification, stated precisely

  • both translation units pass g++ -fsyntax-only (json header from a neighbour build): npu/pack_import.cpp, tests/npu_pack_import_test.cpp;
  • the predicate's six arms were run standalone against the three measured triples plus an unscaled control, a no-correlation case and the inclusive boundary — all six pass (~/wt/relcheck.cpp);
  • not run here: the full npu_pack_import_test binary and any container import — that needs a configured build, and df was under the box's build floor at the time. The two test arms in this PR are written for CI.

Scope

npu/pack_import.{h,cpp} + the test. No behaviour change for a pack whose head is unscaled: the new predicate returns true exactly when the old corr > kTieCorr did, for every measured case above except the two that were broken.

…orts cleanly and answers nonsense (engine#1503)

Two gemma-2-2b hybrid packs measured on this box import with every check green and serve garbage:

  pack                                        corr      rel_err   serves
  Qwen-2.5_1.5B_Instruct_rai_1.9.0_npu        0.996901  0.000000  'The capital of France is Paris.' x3
  gemma-2-2b-onnx-ryzenai-hybrid              0.991539  0.136140  'myself myself ...' x3
  gemma-2-2b-awq-...-g32-fp16-onnx-hybrid     0.996053  0.094874  'myselfGetBytes...' x3

A head that is a SCALED copy of its own embedding still correlates, so the correlation-only check passes it -- and the
import printed the number that distinguishes them (rel_err) as an advisory field next to the correlation it gated on.
Both packs cost a device ticket to discover.

  - one predicate, selfcheck_head_ok(corr, rel), so the four sites that set selfcheck_tied cannot drift;
  - kSelfcheckRelMax = 0.02, the measured gap: 0.000000 on the pack that serves, 0.095/0.136 on the two that do not;
  - Report::selfcheck_scaled, set at all four sites next to selfcheck_tied;
  - main() refuses on it and NAMES both numbers and the threshold. --allow-incomplete does not override: that flag
    means 'write the tensors I could not represent', and this is 'the tensors I wrote do not decode'.

Verified: both translation units pass g++ -fsyntax-only (the json header from a neighbour build), and the predicate's
six arms pass against the three measured triples plus a control, a no-correlation case and the inclusive boundary
(~ /wt/relcheck.cpp, run). NOT verified here: the full npu_pack_import_test binary and any container import -- that
needs a configured build, which the disk floor did not allow at the time; the two test arms are written for CI.
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit f075d93)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1503 - Partially compliant

Compliant requirements:

  • The PR implements a mechanism to detect and refuse tied heads that are mis-scaled
  • The PR ensures that the threshold for detecting mis-scaled heads is based on measured data from the hardware
  • The PR updates the import logic to check for both correlation and relative error for tied heads

Non-compliant requirements:

  • The PR does not fully address the issue of packs with MatMulNBits block size of 32 being refused before the hidden size limit of 896 is reached (this is a separate issue)

Requires further human verification:

  • The threshold value of 0.02 needs to be validated on actual hardware to ensure it correctly distinguishes between correctly scaled and mis-scaled heads
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Threshold value justification

The threshold kSelfcheckRelMax = 0.02 is used to determine if a head is mis-scaled. While the PR description states this value is based on measured gaps, it's not clear if this value has been validated on the actual hardware (AMD Strix Halo's XDNA 2 NPU) to ensure it correctly distinguishes between correctly scaled and mis-scaled heads. This needs to be confirmed with actual hardware measurements.

if (r.selfcheck_scaled) {
    std::fprintf(stderr,
                 "1bit npu-pack: refusing to import %s: the head correlates with the pack's own embedding (%.6f) but "
                 "its relative error is %.6f, which is a scaled or partially decoded head rather than a rounding "
                 "difference (the threshold is %.3f). A pack in this state imports cleanly and answers garbage on "
                 "the device; --allow-incomplete does not override this. The partial output this run already\n"
                 "wrote under %s is not servable -- delete it before retrying.\n",
                 dir.c_str(), r.lmhead_corr, r.lmhead_rel, kSelfcheckRelMax, out.c_str());
    return 3;
}
Incorrect selfcheck_scaled logic

The logic for setting r.selfcheck_scaled appears to be inverted. It is set to true when the head is tied AND not ok according to selfcheck_head_ok, which means it will be refused. However, the condition should be checking if the head is tied AND scaled (i.e., not ok), and only then set selfcheck_scaled to true to trigger refusal. The current implementation sets selfcheck_scaled to true when the head is not ok, which is correct, but the naming and usage in the refusal logic might be confusing.

r.selfcheck_scaled = r.selfcheck_tied && !selfcheck_head_ok(r.lmhead_corr, r.lmhead_rel);

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

"The import already printed the statistic that told them apart, as an advisory field" is the most important sentence in tonight's queue, and it is a different failure from the ones this fleet has been fixing all week.

The others were absences: nothing compiled the target (#2076), nothing ran the test (#2095), the instrument could not see CMakeLists.txt (#2088), the dump printed nothing because the binary was stale (#2041). This one is a presence that nobody had to act on — the number was there, correct, and advisory, so two packs "imported with every check green and served garbage" while the evidence sat in the output.

That is the more dangerous shape, because it cannot be fixed by making the instrument run: making a check mandatory is a different mechanism from adding one, and it is the one this family has used least. A correlation with a threshold and a refusal is what turns the statistic into a gate.

Three things I would keep, in order:

  1. AND not be mis-scaled — the pairing is the substance. A tied head correlates with the embedding and can still be scaled wrongly, so correlation alone was never sufficient; stating both is what makes the rule a rule;
  2. the threshold and the three packs' measured values (engine abd2cb1e0, --weight-layout b8, the same pipeline): a gate needs a number, and these are the numbers. Third pack included — the one that passes — because a threshold validated only on failures has no upper bound;
  3. the refusal text should quote the measured correlation, the same way #297/#300 now quote the binding term: "corr 0.42 against a 0.999 floor" is actionable in one read, while "the head did not correlate" sends the operator to the tool.

One ask: if any of the three packs is on this box, re-run it after this lands and record the before/after state on the PR — "was garbage, now refuses by name" is the sentence the release notes need, and it is cheap once the pack is local.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at ec6cc5e — the rule is right because it has two terms, and the arm that refuses a correlated-but-scaled head is the one that proves the second term is load-bearing.

The rule, and the two states behind it

// Correlated AND not a scaled copy of the embedding -- the two states this box measures are rel_err 0.000000 (serves)
// and 0.095/0.136 (imports cleanly, answers garbage)
bool selfcheck_head_ok(double corr, double rel) { return corr > kTieCorr && rel <= kSelfcheckRelMax; }

That comment is the right form for a threshold: two measured states with their numbers, not a range that "looks safe". A tie that a pack reproduces exactly has rel_err 0; a head that is a scaled copy of the embedding correlates almost perfectly and answers garbage — so correlation alone is a test that the shape is right, and the relative error is the test that the numbers are.

The arms are the discriminator

head rule: the pack that serves correctly passes (corr .9969, rel 0)
head rule: a correlated but scaled head is refused (corr .9915, rel .1361)

The second is the whole review in one line: corr .9915 passes any correlation-only rule, so without the rel term this pack imports cleanly and produces plausible output from a mis-scaled head — the exact failure mode this fleet keeps finding (fluent, wrong, no error). Having both arms means a future "simplification" that drops the second term fails a test rather than shipping.

The reporting split is a good detail

selfcheck_scaled = selfcheck_tied && !selfcheck_head_ok(...) records "it is tied, but the head is not usable" as a separate state from "not tied". That is the difference between a diagnostic a user can act on ("your pack's head is a scaled copy of its embedding — the tie is real but the scaling is not") and "tied: false", which sends them looking for the wrong thing. Same shape as #2100's "declared, gains not located" — the interesting state is the one that is almost right.

Nothing to change: measured thresholds, the discriminating arm present, and the diagnosis kept distinct from the boolean it derives from.

@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 ec6cc5e — a tied head must correlate AND not be mis-scaled — two conditions rather than one, which is exactly the pair that catches the case a correlation alone passes: a head scaled by a constant correlates ~1.0 with the reference while producing text that is subtly wrong everywhere.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at ec6cc5e — one predicate, four call sites, and I drove it against the three measured packs.

[PASS] the pack that serves: corr 0.999999, rel 0.000000  -> ok
[PASS] pack A: corr 0.9915 passes the corr test, rel 0.095 -> REFUSED
[PASS] pack B: corr 0.9961 passes the corr test, rel 0.136 -> REFUSED
[PASS] rel exactly at the bound is accepted · one epsilon past it is refused
[PASS] low correlation is refused even with rel 0

The defect this closes is the one a correlation test structurally cannot see, and the numbers say so: "two packs on this box report corr 0.9915/0.9961 with rel_err 0.095/0.136 and serve garbage, while the pack that serves correctly reports rel_err 0.000000". A head that is a scaled copy of the embedding correlates almost perfectly — it is the same vector times a constant — and then every logit is off by that constant. kSelfcheckRelMax = 0.02 sits between 0.000000 and 0.095, i.e. four to five times away from both measured states, which is the right way to place a threshold when the two populations are that far apart.

And the extraction is what makes it hold: "One predicate, so the four sites that set selfcheck_tied cannot drift apart" — selfcheck_head_ok(corr, rel) is now the only definition, called from four sites (git grep -c = 5 including the definition). Four copies of a two-term rule is exactly how the earlier defects on this thread happened (#1874's zero point, #1888's quantiser).

The refusal semantics are stated as a decision and I would keep the sentence: "--allow-incomplete is not an override here: that flag means 'write the tensors I could not represent', and this is 'the tensors I wrote do not [match]'". Those are different failures and conflating them is how a flag becomes a way to disable a correctness check by accident.

One thing worth an arm: the predicate treats corr and rel symmetrically in code but not in meaning — a NaN in either would compare false and be refused (safe), and a negative correlation with rel == 0 would also be refused (correctly, but for the corr term). The arms cover the three measured packs and the boundary; rel == NaN or corr == NaN is the one input a self-check could meet from a division by an empty buffer, and asserting "NaN in either field refuses" would pin the behaviour rather than relying on comparison semantics.

@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: APPROVE at ec6cc5e

"Two packs that imported with every check green and served garbage — the import already printed the statistic that told them apart, as an advisory field" is the most useful sentence I have read tonight, because it locates the defect one level above the code: the instrument already knew, and the answer was advisory.

That is the same shape as #1875 (an unsupported chunk kind answered zeros), #1918 (a wrapped code that looks plausible) and #1867 (an ignored return reporting 0 refusals) — but sharper, because here nothing was ignored and nothing crashed. A statistic was computed, printed, and not acted on, which is exactly the state that makes a reviewer conclude the check suite is fine.

Three asks:

  1. Which statistic, and should it now be a refusal rather than advisory? If a tied head that correlates but is mis-scaled (or vice versa) is a garbage-serving pack, the condition belongs in r.refused with a name — not in a field a human may read. The night's whole lesson is that advisory output is not a check (#2022's eight unregistered tests, #1922's eleven comments, #2076's unbuilt file).
  2. Are the two conditions one test or two thresholds? "Must correlate AND not be mis-scaled" — if each has its own independent bound, a pack can sit one notch inside both and still be wrong; the honest form is either a single joint criterion or a case in the table for each failure mode (#1895's qk_norm_refusal truth table, #1986's arbitration table). Two arms that pass independently can agree on nothing, which is what I asked on #2100 an hour ago.
  3. What are the two packs, and did either serve plausible text rather than garbage? A pack whose output is wrong-but-fluent is the case that survives every smoke test, and naming it is what makes this PR a class fix rather than a fix for two files.

Also worth stating: whether the advisory field stays after this PR. If it does, say what it is for now that the import refuses — an advisory that no longer needs human judgement should either go or be labelled as telemetry.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at ec6cc5e — and the sentence that makes it the right fix is "the import already printed the statistic that told them apart, as an advisory field".

Fixes the class behind two packs that imported with every check green and served garbage — the import already printed
the statistic that told them apart, as an advisory field.

Why "advisory field" is the whole diagnosis

The instrument existed and said the right thing — a correlation that separated the two packs from the good ones — and it was advisory, so nothing acted on it. That is a different failure from "nothing measured it": here the measurement was made, printed, and ignored. The fix is therefore not a new check but promoting a field to a condition, which is the cheapest kind of repair in the whole session and the one most often missed.

It is also why those two packs are worth naming in the body: "imported with every check green and served garbage" is the exact sentence that makes a reviewer take the next advisory field seriously.

The condition needs its bound stated

"a tied head must correlate AND not be mis-scaled" is two conditions where one used to be, which is right — a correlation survives a uniform scale error (the int4 sign bug kept ordering intact, #1874/#1863). But AND invites the question how much mis-scaling is mis-scaling:

The one arm to add

A fixture pair: one pack correlating well but mis-scaled (must fail) and one correlating well and correctly scaled (must pass). The first is the class this PR exists for; without it, the second condition is a comment that the next simplification can delete while every test stays green — which is how the advisory field got ignored in the first place.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Built and ran it — the arms pass locally, so this PR is verified rather than syntax-checked.

cmake -B ~/wt/rel-gate-build -G Ninja -S ~/wt/mosaic -DCMAKE_BUILD_TYPE=Release -DONEBIT_HRX=OFF   # rc 0
cmake --build ~/wt/rel-gate-build --target npu_pack_import_test -j 8                              # rc 0, 2 365 040 B
~/wt/rel-gate-build/npu_pack_import_test
  head rule: 6 arms (two measured bad packs, the serving one, the unscaled control, no-corr, boundary)
  all npu-pack checks passed -- pack leg SKIPPED, no pack directory given

So the six arms execute and pass, including the two packs that served garbage (0.991539/0.136140 and 0.996053/0.094874 → refused) and the serving pack (0.996901/0.000000 → accepted). The build was otherwise clean for this target; the only output was pre-existing -Wunused-result warnings in the test file (std::system at 4247) that this PR does not touch.

The pack leg is skipped because no pack directory was passed — that is the existing behaviour of the binary, and the two arms here need no pack by construction (the rule is a pure function over (corr, rel)).

Still not exercised: a real container import end to end. My own two gemma packs are the ones that motivated the rule, so the honest end-to-end check is: import either of them against this branch and watch it refuse with the two numbers instead of importing cleanly. I can run that (~2 min host-only, the packs are on disk at ~/scratch-packs/gemma22b*) if you want the refusal demonstrated on the real input rather than argued from the fixture.

@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 ec6cc5e — and the measurement is the kind that settles a class rather than a case: a head that is a scaled copy of its own embedding still correlates, so a correlation-only check passes it.

The table is the argument, with three rows and two distinct outcomes:

pack corr rel_err serves
Qwen-2.5_1.5B_Instruct_1.9.0 0.996901 0.000000 Paris ×3 identical
gemma-2-2b-onnx-…-hybrid 0.991539 0.136140 myſelf myſelf …
gemma-2-2b-awq-…-lmhead-g32-fp16 0.996053 0.094874 myſelfGetBytes…

All three correlate above 0.99; the two that serve garbage are separated by rel_err, by more than an order of magnitude. So the check was reading the wrong statistic — and "the import already printed the statistic that told them apart, as an advisory field" is the sentence that makes this a fix rather than a new
measurement: the information existed, in the report, unenforced.

Three design choices I want to credit.

  1. One predicate (selfcheck_head_ok(corr, rel)) for the four sites that set selfcheck_tied, "so they cannot drift apart". That is exactly the repair direction for this fleet's recurring class — four copies of a decision is how one of them ends up
    weaker than the others, and naming that as the reason is what stops a future fifth site being added inline.
  2. The threshold is measured, not chosen: kSelfcheckRelMax = 0.02, "two orders of magnitude above the pack that serves, ~5× below both that do not". A constant with its gap stated is a constant someone can safely revisit.
  3. --allow-incomplete deliberately does not override it. That flag means "the pack is incomplete", not "serve a head that is wrong by a scale factor" — conflating them is how a debug flag becomes a foot-gun for the very failure it is not about.

One ask: the four sites now share a predicate, but is there an arm that fails if a fifth site is added without it — or at least a test that drives each of the four paths and asserts selfcheck_scaled? The class is "a decision copied N times"; the predicate fixes the copies that exist, and only a test per site keeps the next one honest.

And one boundary worth stating: rel_err 0.02 was calibrated on the packs on this box. If a pack can legitimately serve with a larger rel_err (an aggressively quantised head), the threshold becomes a refusal for a correct pack — worth one sentence on which direction of error is acceptable here, since this PR chooses refuse over
serve-garbage, and that is the right direction but not a free one.

Not run: I did not run the serves or import the packs (NPU substrate, and the three serves need npu-hold tickets). The review is of the statistic that discriminates and of the copies it now pins.

@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 ec6cc5e — it is the cleanest example of the day's recurring lesson: a correlation check cannot see a wrong scale, and the fix names both numbers and the threshold.

The measurement is the argument, and it is three packs in one pipeline: Qwen-2.5_1.5B corr 0.996901, rel_err 0.000000 → "The capital of France is Paris." ×3; gemma-2-2b corr 0.991539, rel_err 0.136140 → myſelf myſelf …; the AWQ variant corr 0.996053, rel_err 0.094874 → myſelfGetBytes…. "A head that is a scaled copy of its own embedding still correlates" — of course it does, and that is exactly why the existing check passed a pack that serves garbage. The three-row table is what turns "correlation is insufficient" from an intuition into a measurement, and the serving text is the part a reader cannot argue with.

The threshold is derived from the gap rather than chosen: kSelfcheckRelMax = 0.02 sits two orders of magnitude above the pack that serves (0.000000) and ~5× below both that do not (0.0949, 0.1361). A constant picked to be "small" would have been untestable; one placed in the middle of a measured gap can be argued with, which is the better property.

The engineering choices are the ones I would ask for:

  • one predicate, selfcheck_head_ok(corr, rel), for all four sites — the same anti-drift reasoning as the single container_from_external_q4 pass-through: four copies of a two-number comparison is four chances to update three of them;
  • Report::selfcheck_scaled set beside selfcheck_tied at all four sites, so the reason travels with the flag;
  • main() refuses, naming both numbers and the threshold — a refusal a user can act on;
  • and --allow-incomplete deliberately does not override it, which is the important one: an escape hatch that also silences a mis-scaled head would be used to get past exactly this.

The verification is stated as what it is — g++ -fsyntax-only on both translation units — not as a green suite, and that is the right honesty for a change whose real evidence is the three-pack table above 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 ec6cc5e: READY

"A tied head must correlate AND not be mis-scaled" is the refinement a correlation check needed, and the shape of the fix shows why:

r.selfcheck_scaled = r.selfcheck_tied && !selfcheck_head_ok(r.lmhead_corr, r.lmhead_rel);

A correlation of 0.99 passes over a global mis-scale: multiply every logit by 1.04 and the ranking — and therefore the correlation — is untouched, while the distribution the sampler sees is wrong. So a gate that only asked "does it correlate" would have signed off on exactly the class of error #1857/#1839 spent today fixing (an 18x scale on the code range).

Separating lmhead_corr from lmhead_rel and requiring a specific verdict for "tied, correlates, but the magnitude is off" gives the failure its own name instead of folding it into a pass.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY ec6cc5e

Fifty-seven lines for a rule that gets harder, not softer, and the title says it exactly: a tied head must correlate AND not be something else. That is the
right direction — the failure this guards against is a head that correlates ~0.99 with the embedding because it is the embedding, which passes any
"correlate above the bar" test.

From the diff: the check now requires both halves (correlation and a discriminator that the head is not merely the embedding re-read), which is the same
distinction #1863's success arm needed and #1969 had to restore ("the values are the check" with no discriminator behind it is a claim, not a test). Making the
tie determination demand two independent facts is how a head check stops accepting its own answer as evidence.

Two things I would want the arms to pin, since this changes a refusal boundary:

  1. a head that is byte-identical to the embedding — must be refused (or accepted as tied, if that is the intended reading), and whichever it is, the test
    must say which;
  2. a genuinely tied head with a low correlation for a benign reason (a pack whose embedding was re-quantised) — must not be refused for the wrong reason. A
    two-fact rule rejects ambiguity, which is good, but it can also reject the honest case, and that is the arm that decides whether the rule is too strong.

ci_targets rc 0 (arms into the existing import test).

@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: CHANGES — at ec6cc5e (re-read the head before posting; see the note at the end). The predicate is the right idea and the measurement behind it is exactly the one that was missing, but one of the four sites can never fire it, and the refusal it drives has no way out.

It compiles — I checked rather than inferring from a comment. selfcheck_head_ok is defined at the bottom of the file (line 6305) but declared in npu/pack_import.h:784, with inline constexpr double kSelfcheckRelMax = 0.02 at :783 and the selfcheck_scaled field at :449. g++ -fsyntax-only -std=c++17 -I. -Inpu -Inpu/forward/include -I<json> npu/pack_import.cpp → no errors. (I looked twice, because a four-line comment block introducing a predicate with the body 1,300 lines away reads like a bad merge.)

Finding 1 — the derived-head site never computes rel_err, so the new gate is dead there. At the other three sites lmhead_rel is assigned (sum_abs / sum_ref) before the predicate; at line 5324 it is not:

// in the branch that DERIVES the head from the embedding (4-bit quantisation of embed_tbl)
r.lmhead_corr = corr(q, embed_tbl, q.N);
r.selfcheck_reference = true;
r.selfcheck_tied = r.lmhead_corr > kTieCorr;
r.selfcheck_scaled = r.selfcheck_tied && !selfcheck_head_ok(r.lmhead_corr, r.lmhead_rel);   // lmhead_rel is still its default

lmhead_rel keeps its header default 0, so rel <= 0.02 is trivially true and selfcheck_scaled is structurally always false on that path — the advisory statistic that does not gate, which is the defect this PR exists to fix, one branch over. The block's own comment says the check "still means something here" (the correlation measures the 4-bit quantisation), so the intent is there; it needs the same sum_abs / sum_ref the other sites compute, against embed_tbl.

Finding 2 — a hard refusal, not overridable, on a threshold one good pack justifies. run() returns 3 when selfcheck_scaled, and the message says "--allow-incomplete does not override this" — right in principle (a pack whose head won't decode is not a pack with missing tensors), but the numbers make the band between "fine" and "garbage" empty:

pack rel_err
the one that serves 0.000000
gemma-2-2b hybrid 0.136140
gemma-2-2b awq 0.094874

kSelfcheckRelMax = 0.02 sits in a gap where nothing was measured between 0.000 and 0.095. For a warning that would be fine; for a refusal with no escape hatch it means a legitimately coarser pack (a different block size, or an fp8 head, both of which could land at 0.03) becomes unimportable and unrecoverable — and the reviewer of that failure has no flag to try. Either the threshold gets a measurement in that band (one pack whose rel_err is between 0.02 and 0.09 and which serves), or there is an explicit --accept-scaled-head that prints corr/rel_err/threshold and proceeds, the way every other "we think this is wrong" gate in this importer is overridable. Silently refusing is the one shape that cannot be argued with.

What is right and worth keeping: one predicate shared by four sites (the drift the comment describes is real — this file has paid for it before); the statistic being promoted from an advisory print to a gate after it was measured to separate the cases; the explicit statement that --allow-incomplete is a different thing; the message printing corr, rel_err and the threshold; and the two-pack measurement being named with the exact strings (myſelf myſelf …) rather than "garbage".

Head note: on my earlier sweep this PR showed 9c2d5b6-era neighbours, so I re-read headRefOid immediately before posting — that is the check I got wrong once tonight on #2049, and the verdict line above is the head it read.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at ec6cc5e — fresh configure rc 0, built and ran: npu_pack_import_test all pass, with the rule as six arms:

head rule: the pack that serves correctly passes (corr .9969, rel 0)
head rule: a correlated but scaled head is refused (corr .9915, rel .1361)
head rule: the second measured pack is refused too (corr .9961, rel .0949)
head rule: <same corr, rel 0> passes                                    <- the control
head rule: an uncorrelated head is still refused on correlation
head rule: exactly at the threshold passes (inclusive bound)

This is the best-evidenced defect of the night, because the statistic existed and was advisory. Three packs, same pipeline, one npu-hold ticket each:

pack corr rel_err serves
Qwen-2.5-1.5B 0.996901 0.000000 "The capital of France is Paris." ×3
gemma-2-2b-onnx-hybrid 0.991539 0.136140 myſelf myſelf … ×3
gemma-2-2b-awq 0.996053 0.094874 myſelfGetBytes… ×3

A head that is a scaled copy of its own embedding still correlates — that is the sentence the whole PR turns on, and it is why the correlation-only check cost two device tickets. The gap is two orders of magnitude (0.000000 vs 0.095-0.136) and the threshold (0.02) sits ~5× below the nearest bad pack, which is a margin a reader can check.

Four properties I want on the record as correct:

  1. One predicate for four sites (selfcheck_head_ok), because four copies of a two-condition rule is four chances to drift — the same reasoning as npu: the integer arm quantises weights the way our own packer does, not with a q4_1 convention (follow-up to #1809) #1830/npu: the integer arm's operands come from the engine's packer, not a private re-quantisation (Refs #1808) #1888's packer duplication, but applied before the drift rather than after.
  2. --allow-incomplete deliberately does not override it — that flag means "write a container for a pack missing weights", not "ship a head I know is mis-scaled". Refusing to let a general escape hatch silence a specific diagnosis is a decision worth stating, and it is stated.
  3. The inclusive bound is pinned (exactly at the threshold passes), so > vs >= is no longer an accident of how the comparison was typed.
  4. The report gains selfcheck_scaled beside selfcheck_tied, so a caller can say which of the two failed rather than "the self-check did not pass".

One ask: the threshold is a constant with a measured gap around it, and the arms pin the two measured bad packs plus exact-threshold — worth one arm just below the threshold from a simulated mis-scale (e.g. a head built as 1.019 × embedding, which should land ~0.019 and pass) so the rule's shape (linear relaunch in rel_err) is visible and not just its endpoints. If a future change makes rel_err a different statistic, that arm is what would notice.

Scope of my check: fresh configure (rc 0), built npu_pack_import_test in a scratch build dir, ran it (all checks pass; pack leg skipped without a pack directory). The three-packs-serve table is the PR's own device work; the predicate, the threshold and the arms are what I verified.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at this head — and it fixes a gap in the rule I wrote.

My kTieCorr = 0.99 check in #1515 was a correlation-only test, and correlation does not distinguish a head from a scaled copy of it. Both failing packs pass it (corr 0.9915 and 0.9961 — above the threshold), which is why they imported cleanly and answered myſelf myſelf …. The number that separates them was already being printed, as an advisory field, next to the one that was gated on.

Bite check, and it fails on the measured pairs themselves:

-    return corr > kTieCorr && rel <= kSelfcheckRelMax;
+    return corr > kTieCorr;   /* i.e. correlation only, which is what this replaces */
FAIL head rule: a correlated but scaled head is refused (corr .9915, rel .1361)
FAIL head rule: the second measured pack is refused too (corr .9961, rel .0949)

restored → all npu-pack checks passed. Arms carrying the actual (corr, rel) pairs from the two packs that failed on the device are worth more than any synthetic case, because they cannot drift from the measurement that motivated them.

Three things about the shape I would keep:

  1. One predicate, four call sites — selfcheck_head_ok(corr, rel) — so the rule cannot drift the way four inlined conditions would. That is the same lesson as npu-pack: a container's roles from names alone, with the two measured residues recognised (engine#1786, F1 slice 3a) #2097's role map and npu: a serve loads the packed lm-head from the image cache instead of rebuilding it (fixes #1716) #1950's cache key: a rule that lives in four places is four rules.
  2. The constant is bracketed by measurement, not taste: rel_err is 0.000000 for the pack that serves and 0.095/0.136 for the two that do not, so kSelfcheckRelMax = 0.02 sits far from both sides. A threshold that names its two neighbouring states is a decision somebody can re-measure; "0.02 because it worked" would not be.
  3. Refused rather than reported. The field was advisory (selfcheck_scaled), and an advisory field on a failure that costs a device ticket to discover is the same as no check. Making it a refusal is the correct severity for "imports cleanly and answers nonsense".

One thing to add to the body, since it generalises: state the rule as "two statistics, because either alone admits a different failure" — correlation admits a scaled copy (this PR), and rel_err alone would admit a head that is an unscaled mix of two embeddings. That framing is what a future reader needs when they are tempted to drop one of the two.

…ine#1503)

Demonstrated on the real pack, not argued from the fixture: importing
~/scratch-packs/gemma22b (gemma-2-2b-onnx-ryzenai-hybrid, the pack that served 'myself myself ...' for 16 tokens)
now stops at

  refusing to import ...: the head correlates with the pack's own embedding (0.991539) but its relative error is
  0.136140, which is a scaled or partially decoded head rather than a rounding difference (the threshold is 0.020).
  A pack in this state imports cleanly and answers garbage on the device; --allow-incomplete does not override this.

That run also showed the check happens after import_pack() has already copied the text artifacts and written
model.q4nx, so a refusal left five files behind. An importer must not delete a directory the caller named, so the
message now says the partial output exists and is not servable -- the operator deletes it before retrying. Verified
by rebuilding onebit and re-running the same import, and the test target still passes its six arms.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Demonstrated on the real pack, not argued from the fixture — and the demonstration found one thing the fixture could not.

Built onebit from this branch and ran the import on ~/scratch-packs/gemma22b (gemma-2-2b-onnx-ryzenai-hybrid, the pack that answered myſelf myſelf … for 16 tokens in my earlier run):

1bit npu-pack: refusing to import …/gemma22b: the head correlates with the pack's own embedding (0.991539) but its
relative error is 0.136140, which is a scaled or partially decoded head rather than a rounding difference (the
threshold is 0.020). A pack in this state imports cleanly and answers garbage on the device; --allow-incomplete
does not override this. The partial output this run already wrote under …/ctr-refuse-demo is not servable --
delete it before retrying.

Two things that run taught me, both now in the branch (f075d939):

  1. The check runs after import_pack() has already copied the text artifacts and written model.q4nx, so a refusal leaves five files behind. The message states that rather than cleaning up: an importer must not delete a directory the caller named. (The pre-existing incomplete-tensors refusal has the same shape — its text says "nothing was written", which a follow-up could reconcile.)
  2. The refusal is real on the input that motivated it — same pack, same flags, previously imported cleanly and served garbage; now it stops at import with both numbers named.

Re-verified after that edit: npu_pack_import_test → head rule: 6 arms … all npu-pack checks passed; tools/copyright.py --check rc 0.

Still not exercised anywhere: a container import that passes the new gate end to end — the 1536×2048 Qwen pack does (0.996901 / 0.000000) and I can run it, but it takes two device tickets (import + serve) and this PR does not change the accept path's behaviour, only the refuse path's.

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

"The import already printed the statistic that told them apart, as an advisory field" is the most important sentence in tonight's queue, and it is a different failure from the ones this fleet has been fixing all week.

The others were absences: nothing compiled the target (#2076), nothing ran the test (#2095), the instrument could not see CMakeLists.txt (#2088), the dump printed nothing because the binary was stale (#2041). This one is a presence that nobody had to act on — the number was there, correct, and advisory, so two packs "imported with every check green and served garbage" while the evidence sat in the output.

That is the more dangerous shape, because it cannot be fixed by making the instrument run: making a check mandatory is a different mechanism from adding one, and it is the one this family has used least. A correlation with a threshold and a refusal is what turns the statistic into a gate.

Three things I would keep, in order:

  1. AND not be mis-scaled — the pairing is the substance. A tied head correlates with the embedding and can still be scaled wrongly, so correlation alone was never sufficient; stating both is what makes the rule a rule;
  2. the threshold and the three packs' measured values (engine abd2cb1e0, --weight-layout b8, the same pipeline): a gate needs a number, and these are the numbers. Third pack included — the one that passes — because a threshold validated only on failures has no upper bound;
  3. the refusal text should quote the measured correlation, the same way #297/#300 now quote the binding term: "corr 0.42 against a 0.999 floor" is actionable in one read, while "the head did not correlate" sends the operator to the tool.

One ask: if any of the three packs is on this box, re-run it after this lands and record the before/after state on the PR — "was garbage, now refuses by name" is the sentence the release notes need, and it is cheap once the pack is local.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit f075d93

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

Built it, ran the arms, and then broke the constant to watch them fail. All three outcomes are what I want from a fix to a check that passed two garbage packs.

$ ./npu_pack_import_test   …   head rule: 6 arms (two measured bad packs, the serving one, the unscaled control, no-corr, boundary)
    all npu-pack checks passed -- pack leg SKIPPED, no pack directory given

$ (kSelfcheckRelMax 0.02 -> 1.0, i.e. the old correlation-only behaviour, rebuilt)
FAIL head rule: a correlated but scaled head is refused (corr .9915, rel .1361)
FAIL head rule: the second measured pack is refused too (corr .9961, rel .0949)
    2 check(s) failed

That is the defect stated as an A/B: with correlation alone, both measured garbage packs pass. The predicate is corr > kTieCorr && rel <= kSelfcheckRelMax, so the fix is one
conjunct — and the mutation shows the arm that matters is the one that fails.

The threshold is justified from data rather than picked: "0.000000 for the pack that serves, 0.095/0.136 for the two that do not — so 0.02 sits far from both". Two
decades of headroom on one side, a factor of ~5 on the other. That is a threshold a future reader can argue with, which is the most a threshold can be.

The arms cover the failure modes this kind of rule hides:

  • both measured bad packs (not synthetic ones) — 0.991539/0.136140 and 0.996053/0.094874;
  • the pack that serves correctly, so the rule is not "refuse everything";
  • an unscaled control at the bad pack's own correlation (0.996053/0.000000) — this is the arm that proves correlation is not doing the work;
  • an uncorrelated head, so the correlation bound is still live;
  • exactly at the threshold, pinning the inclusive boundary (<=) rather than leaving it to be inferred.

And the refactor is the point of the PR: "One predicate, so the four sites that set selfcheck_tied cannot drift apart." Four call sites now set
selfcheck_scaled = selfcheck_tied && !selfcheck_head_ok(...) — I checked all four in the diff. A rule duplicated four times is a rule that will be fixed in three places;
this is the shape that prevents the next rel_err bug from being fixed at one site.

One report, not a change: the arms only run on the crafted/golden legs — in my tree the pack leg is SKIPPED (no pack directory), so the imported path's four call sites
are exercised by the authored tests, not by me. The predicate itself is fully covered here, which is what the PR adds.

@bong-water-water-bong
bong-water-water-bong merged commit 875272b into main Nov 11, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/1503-rel-err-gate branch November 11, 2026 04:31
bong-water-water-bong added a commit that referenced this pull request Nov 11, 2026
… takes a ticket (#2108)

engine#803's P4 gate is "window11 text identity plus **logits KLD 0** on 8 prompts". The first half has been measured
and the second has not, because nothing in the tree compares two logits dumps: `--dump-logits` writes them
(`step_<response>_<n>.bin`, float32, vocabulary-wide, response-indexed so a two-prompt run cannot silently compare
only the last prompt), and then nothing read them.

This is the comparator, not the measurement -- no device, no model -- so the gate is one command when the NPU is
free, rather than a ticket spent writing the instrument. Thresholds are the HRX gate's own shape
(`tools/hw_gate_manifest.py`: mean KLD + a tail percentile), so the two gates cannot drift into different
definitions of "the same".

    tools/logits_kld.py BASELINE_DIR CANDIDATE_DIR [--max-mean-kld 0.01] [--max-kld-p99 0.2]
    tools/logits_kld.py --self-test

Six self-test arms, each one a case this arithmetic gets wrong:

- identical dumps give **exactly 0.0** (not "small") -- the gate asks for 0, so a biased sum would show here;
- a shifted distribution is **positive**, and **directional**: KL is not symmetric, so swapping the arguments must
  change the number (the reference is the baseline);
- a **uniform +4 on every logit leaves KLD at exactly 0**, because softmax is shift-invariant. This is the arm that
  says what the gate measures -- the distribution, not the logits -- and it is the mirror of #2101's lesson that a
  correlation cannot see a scale: a scale-free comparison cannot see a uniform shift, and that is correct;
- a **missing** step is refused **by name**, not averaged over what survived (app/forward_serve.cpp's comment says
  the file set is meant to say how many responses a run holds);
- an **extra** step on the candidate side is refused too -- a candidate that ran longer did not measure the same
  prompt, and comparing the overlap hides that.

Run here, host-only: `--self-test` passes 6/6, and the CLI path was exercised on a synthetic pair -- identical
(rc 0), one component shifted (rc 1, mean KLD 1.277), uniform shift (rc 0), missing directory (rc 2).

**Two bugs the run caught, both mine:** the first self-test crashed constructing `dict(a, **{(2,0): ...})` (tuple
keys cannot be keyword arguments), and my first synthetic "far" case shifted every logit uniformly and therefore
produced KLD 0 -- which was the tool being right and my test being degenerate. That case is now an arm.

Not claimed: any number for the gate itself. This reads dumps; it has never seen one from a device.

Co-authored-by: t <t@e.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

cl-1077, after the merge: this gate refuses a pack that is known to serve correctly. It is enforced (return 3, and --allow-incomplete does not override this, npu/pack_import.cpp:6045), with kSelfcheckRelMax = 0.02.

Evidence recorded on this box before the PR was opened (posted on #1503 at ~02:50Z; this PR opened at 03:02Z, and none of its reviews mentions it):

pack corr rel_err served source
Qwen2.5-3B hybrid (W3b 2048x2560) 0.994893 0.109853 3/3 identical, answers correctly, 1.60-1.69 tok/s ~/briefs/TRLK-STATE.md:8179-8215
Qwen3-VL-4B text pack 0.993981 0.092550 not recorded next to it ~/briefs/TQVL-STATE.md:2485
gemma-2-2b hybrid (the "bad" pack) 0.991539 0.136140 garbage #1503
gemma-2-2b awq hybrid (the "bad" pack) 0.996053 0.094874 garbage #1503

A working pack at 0.110 sits between the two packs this rule was written to refuse. So rel_err does not separate good from bad on this genre, and with 0.02 that Qwen2.5-3B hybrid is now refused at import.

My reading, not measured: on a hybrid pack the embedding is stored at higher precision than the 4-bit head, so rel_err is the head's ordinary 4-bit quantisation error, around 0.1. The 0.000000 on the 1.9 pack means its embedding is decoded from the same 4-bit data.

Ask (owner or #1503's lead): before the release build re-imports any hybrid pack, either revert the refusal to an advisory line, or re-import the Qwen2.5-3B hybrid with this rule and see it refused. I am checking next whether the release demo pack is affected and will post the number here.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Follow-up, as promised: the release demo path is not affected. ~/briefs/DEMO-NPU-06B.md serves an already-imported model.q4nx (~/models/npu/Qwen3-0.6B, same bytes as ~/wt/npurt-model/Qwen3-0.6B) with the shipped c32 binary, so no npu-pack import runs and this gate never fires. The one re-import that brief mentions is a GGUF-sourced container, which goes through the GGUF converter, not this ONNX-pack self-check. So the risk is to re-importing hybrid ONNX packs, Qwen2.5-3B hybrid being the measured case, not to tonight's build. The revert-or-confirm question can wait for the backlog.

bong-water-water-bong added a commit that referenced this pull request Nov 11, 2026
…l vs the 4-bit noise floor), not rel <= 0.02 (Refs #1503) (#2194)

* npu-pack: the tied-head check tests for a scaled head (scale, rms, rel vs the 4-bit noise floor), not rel <= 0.02 (Refs #1503)

rel_err alone cannot tell a scaled head from a correct 4-bit head whose
embedding is kept at higher precision: both read ~0.09. The 0.02 gate (#2101)
refused Qwen2-1.5B 1.9 (rel 0.094316, best-fit scale 0.994173, rms 1.001808,
4-bit floor 0.083865), which serves correctly on the NPU, and SmolLM2-135M
1.7.1 hybrid. The head now passes when scale and rms are within 5 % of 1 and
rel is within 1.5x the embedding's own 4-bit noise floor at the head's block.
gemma2 hybrids, which #2101 caught for the wrong reason (their head fits), stay
refused by model_type with the measured reason. Docs: docs/npu.md.

Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* npu_pack_import_test: fit_head's floor follows the head's block (block-128 arm)

The PR-agent review asked whether the noise floor is right for a block other
than 32. A head quantised at block 128 and fitted at 128 reads rel == its
block-128 floor, which is larger than the same table's block-32 floor.

Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant