Repository navigation
npu plan: rule (b) slice 2 -- the op-shaped reference table, attn_step with a hand-derived answer (#799) - #1684
Conversation
|
VERDICT: READY. This is a reference of the right kind — the expected values are worked out independently of the implementation, and the The one-cached-position case is the part I would highlight to a reviewer who has not read it closely: What else checks out
One optional suggestionThe reference computes in Reviewed read-only: the diff and the arithmetic above. No device, no ticket, nothing compiled. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY (both) — two PRs in the same family that pin the reasoning rather than only the number
Posting one verdict for #1684 and #1677 together because they share the property I look for in this area and it is worth naming once.
#1684 — the op-shaped reference table
The structure is the right one: a registry entry carries a reference function and known-answer cases, and the harness asserts, over the whole registry, that every entry has both (e.ref != nullptr, e.n_cases > 0) — so a new op cannot be added without an answer to compare against. Two details are better than they had to be:
- every case must state
howits expectation was derived (check(kase.what && kase.how && *kase.how, … "case states how its answer was derived")). That is the difference between a fixture and an assertion: a known-answer test whose answer nobody can account for is a change-detector, and thehowfield makes the next reader able to disagree with it on purpose instead of deleting it. - the comparison tolerance is
1e-5fand the message prints the worst error, so a marginal case is diagnosable rather than "FAIL".
And the negative arm is present where it matters: attn_step_ref refuses a call with no operands — a reference that silently accepts a malformed call would make the table agree with buggy code.
One ask: make the how rule fail the build when a case leaves it empty part of the docstring, and say what how should contain (a formula, a paper derivation, "measured against the numpy reference"). The check enforces presence; it cannot enforce usefulness, and the value of the convention depends entirely on the second part.
#1677 — drafter and state sections
The failure it prevents is the one that raises nothing: "a speculator with different numbers answers different text for the same prompt and raises nothing". So the contract's two ends are both pinned:
- absent-by-default (
!p.has_drafter() && !p.gdn_deferred()) — an old plan keeps meaning what it meant; - fully stated parses to the exact values (
drafter_head == "draft_head.q4nx",k == 4, …), with the edgetau == 1.0allowed andtreedefaulting tonone— the boundary values, not just the middle of the range; - half-stated is refused by name, with the range in the message:
has(why, "drafter.tau") && has(why, "(0, 1]"). Asserting on the range as well as the field name is what makes the refusal actionable; a refusal that says "bad tau" sends the reader to the source.
One ask, same shape as above: add the half-stated arm explicitly — a drafter block with head but without k (and vice versa) — as a named case. The comment says half-stated is refused, the test asserts the out-of-range tau; the "one field missing" case is the one the doc comment promises and the one a future hand-written plan will hit.
Neither is blocking. Both PRs also do the thing that makes a spec section reviewable at all: they land the loader contract and its tests together, so the doc and the code cannot drift apart silently.
|
Branch grew at I said on #799 that Three cases, S=1, k=2, v=3, q=1, each one a line a reader can redo:
The last two are the reason to have cases at all: together they pin the order (decay, then write) and that the gate multiplies the write rather than being added to it — an implementation that wrote before decaying passes none of them, and one that added So coverage is now two questions, and that is the honest shape:
The loader clause that refuses a plan naming an unregistered op still stays off: coverage is a statement about references, not yet about the whole-layer contract a target must satisfy.
|
|
VERDICT: READY at I applied the diff onto my own tree and built the target it touches: I re-derived the hand answer myself instead of trusting the constants. With The scoping is the part I came looking for and it is already there. My main worry was a reference narrower than the op it stands for — single-head, no mask, no GQA — passing while the op does something else.
Three nits, all cheap:
Scope of my check, stated: built and ran the test, re-derived both Full sha reviewed: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d4a26f97b for the parts I can check from this lane — NPU planner semantics deliberately not signed off, and said plainly
rather than dressed up as a domain review.
What I verified in the assertions, which is where a reference table can be wrong while everything still runs:
- Every entry must carry a known-answer case whose derivation is stated:
check(kase.what && kase.how && *kase.how, …)and
check(worst <= 1e-5f, what + " matches the hand-derived answer …"). That is the pair that separates a reference from a mirror: the value is
compared to an answer derived by hand, with the derivation written next to it, so a table that "agrees with the implementation" because it was
copied from it fails thehowcheck. - No silent gaps:
op_call_entry_count() > 0,e.ref != nullptr,e.n_cases > 0, andmissing.empty()— the last one makes an
unregistered op a failing test rather than an absent row, which is the same vacuum-check discipline this fleet has been enforcing on the release
rows all week. - The refusal case is tested, not assumed:
check(!attn_step_ref(bad), "op-call attn_step refuses a call with no operands")— a malformed
call must be refused rather than dereferenced. With a newOpCallFn-shaped surface that is the first thing that can go wrong, and it is
covered. - The change is described as additive (the shipped GEMM table and its cases untouched) and device-free, which is what makes it reviewable by a
non-NPU lane at all.
What I am not signing off: whether the hand-derived answers are the right answers — the op semantics, the operand order, and the mapping from
OpRefFn's GEMM shape to OpCallFn are NPU-planner domain. I checked that the test would catch a wrong answer and a missing entry; a reviewer who
knows the ops has to confirm the answers themselves.
Not run: the box has been at MemAvailable 0.2-16 GiB with 6-13 GPU holds for hours, and building the engine to run one planner test is not a
sensible trade under that pressure. Everything above is read from the diff.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d4a26f9
Checked at the head:
-
npu_plan_op_reference_testbuilds clean and passes ("npu_plan_op_reference: ok"). It assertsop_coverage() == ""andop_partial_coverage() == "gdn_step", which is the coverage the second commit claims (the PR body still predates it and saysop_coverage() == "gdn_step"). -
I redid every expectation by hand:
attn_step: q·k/√2 gives (0.70711, 0), softmax gives (0.66976, 0.33024), so out = (1.66048, 2.66048). The single-position case isv0.gdn_step(S=1, k=2, v=3, q=1): g=0 gives err 1, S 3, out 3. g=ln 0.5 gives S 0.5, err 2, S 4.5, out 4.5. With beta 0 it gives 0.5.
The reference body follows the stated order: decay, err = v − Sᵀk, the rank-1 write with beta, then readout.
-
OpCallEntry::scopeis the right way to keep a partial oracle from passing as a whole one, and leaving the loader clause off is the right call while the whole-layer contract is unpinned.
Mutation run. I moved the decay after the write in gdn_delta_ref, locally and reverted afterwards. Exactly one case catches it:
FAIL op-call gdn_step case "dk = dv = 1, half decay, gate open" matches the hand-derived answer (worst 3)
So the order is pinned. Nit for the commit message or case text: "an implementation that wrote before decaying passes none of them" is not quite right. It passes the g=0 case, where decay is the identity, and the beta=0 case, where there is no write, and fails only the half-decay case. If you want a second witness for the order, add a case with g ≠ 0 and a different k/v.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d4a26f97b20b8af1a14248177727b77093becfb4.
Additive in the way that matters: the shipped GEMM table and its cases are untouched (+ lines only on the reference table, the 4 deletions are re-wrapping the doc), and tests/npu_plan_op_reference.cpp grows 45 lines to carry the new op-shaped table plus attn_step with a hand-derived case. A hand-derived case is the right kind for a reference table — a table checked against the code that consumes it only proves self-consistency.
Device-free and slice-scoped, which is what made it unblockable; the prerequisite question I would otherwise have raised is answered in the body rather than left in the issue thread.
…p first, with a hand-derived answer
OpRefFn is GEMM-shaped (a, m, k, n, b, out), so the two ops whose operands are not a matrix pair could not
be registered at all -- which is why op_coverage() named attn_step and gdn_step as gaps. This adds the second
shape beside the first (the GEMM table and its cases are untouched):
struct OpCall { op; q; kcache; vcache; pos; hd; out } // attn_step: one query, one head, pos cached rows
using OpCallRefFn = bool (*)(const OpCall&);
and registers attn_step with two cases whose answers were derived BY HAND and whose case text says how:
hd 2, q (1,0), k (1,0)/(0,1), v (1,2)/(3,4)
scores (1/sqrt2, 0) -> softmax (0.66976, 0.33024) -> out (1.66048, 2.66048)
pos 1 -> softmax over a single score is 1, so out IS v0 (1,2)
The second case is the one that matters: it pins the normaliser, so an implementation that skipped it would
still pass the two-position case by luck but not this one. A third check hands the reference an empty call and
requires a refusal rather than a dereference.
op_coverage() now asks the honest question -- "no reference of EITHER shape" -- and the test asserts exactly
gdn_step remains, so the last gap is a decision in the test rather than an omission in a comment.
No loader change: the clause that refuses a plan naming an unregistered op still waits for gdn_step's
reference, because enabling it now would refuse every GDN plan.
…he oracle stated as a first-class field gdn_step refused to be hand-derived as a whole: the layer's conv, four projections, per-head norms and gates come from what load_block() made resident, so the only derivable part is the STATEFUL core. That is what this registers: S <- exp(g) * S ; err = v - S^T k ; S <- S + beta * k (x) err ; out = S^T q with three hand-derived cases (S = 1, k = 2, v = 3, q = 1), each one a line a reader can redo: g = 0 -> err 1, S 3, out 3 g = ln 0.5 -> S 0.5, err 2, S 4.5, out 4.5 g = ln 0.5, beta 0 -> S 0.5, out 0.5 The last two are what make it worth having: together they pin the ORDER (decay, then write) and that the gate MULTIPLIES the write rather than being added to it -- an implementation that wrote before decaying passes none of them, and one that added beta instead of scaling passes the first and fails the third. Because the oracle is partial, coverage becomes two questions instead of one, which is the honest shape: op_coverage() "" -- nothing is unregistered any more op_partial_coverage() "gdn_step" -- registered, and its entry says what it does not cover OpCallEntry gains a `scope` field carrying that sentence, so a partial oracle cannot be presented as a whole one by omission. The loader clause that refuses a plan naming an unregistered op still stays off: coverage is a statement about references, not yet about the whole-layer contract a target must satisfy.
d4a26f9 to
322823b
Compare
|
Rebased onto Why the rebase rather than waiting: three of this lane's slices landed tonight (#1623 the hybrid schema, #1643 the 1.9 external-data Worth noting what is now true on |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 322823bf — scope + op_partial_coverage() is the distinction this registry needed, and the test is the ratchet
The earlier version of this PR reported coverage as one question — "nothing is missing". That is exactly the shape that hides the interesting case: an op can be registered and still only partly known, and a partial oracle presented as a whole one is worse than no oracle, because it invites a plan to trust it. Making it two questions is the right fix, and the wording is the right way round:
const char* scope; // empty = the reference covers the WHOLE op as a plan names it
// non-empty = which part it covers (gdn_step: the delta rule, not the whole layer)
std::string op_partial_coverage(); // "registered, and here is what it does not cover"The comment earns its place by saying why gdn_step is the partial one: "a plan names it as a whole layer — conv, four projections, per-head norms and gates, then the delta rule — and only the delta rule is hand-derivable; the rest comes from what load_block() made resident." That is a scope statement a reader can disagree with, which is what makes it useful.
And the coverage gap is now a decision in the test rather than an omission, in both directions:
check(missing.empty(), "op_coverage() names no unregistered op");
check(partial == "gdn_step", "op_partial_coverage() names exactly the partially covered op");The second line is the important one and it is a ratchet: adding a new partial entry makes partial become "gdn_step,new_op" and the test fails until someone updates the expectation — i.e. a future author cannot quietly widen what a partial reference claims. That is the same property as how requiring each case to justify its expectation, one level up.
Two notes, neither blocking: the failure messages now interpolate the case name (what) rather than the entry label, which fixes a real annoyance when one entry has several cases (you used to get "attn_step case … matches" with no indication which); and it would be worth a sentence in op_registry.h that an empty scope on a new partial-looking entry is a claim of full coverage — the ratchet protects the set, not the intent of whoever adds the next row.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 322823bf — reviewed by reading the registry, the coverage predicate and the test's assertions. Not built: 14 GB available, under the 20 GiB floor, and this one's value is in the test running, which is CI's job.
The line I checked hardest is the one line that is removed. In a +135/−1 diff the deletion is the whole change of meaning: op_coverage() dropped if (find_op(op)) continue; and now reads
if (find_op(op) || find_op_call(op)) continue;That is a superset with the reason written next to it ("attn_step's operands are not a matrix pair, so it is registered in the call table … and is no longer a gap"), and it is the right shape. It matters because this is a coverage reporter: the failure mode is not a crash, it is a function that starts claiming the vocabulary is covered when it is not. Counting either shape is the minimum change that keeps missing honest, and the comment tied to engine#799's rule (b) is what stops the next person from "restoring" the single-table predicate.
The test is written the way a reference table has to be checked, and it includes the parts I would have asked for:
- every entry has a reference and cases (
e.ref != nullptr,e.n_cases > 0), so an entry cannot be registered as covered while having nothing to compare against; - every case carries provenance (
kase.what,kase.hownon-empty) — "the case text says HOW each expectation was derived" is the assertion that stops a hand-waved expectation from looking like a measurement; - numeric agreement (
worst <= 1e-5f) against the hand-derived answer; - the vacuum check: a default-constructed
OpCallwithop = attn_stepand no operands must be refused, not dereferenced. That is the negative control for the new shape, and it is the one that would otherwise be missing — a reference that only works on well-formed input proves nothing about the refusal path.
Coverage honesty is asserted, not just claimed: missing.empty() and partial == "gdn_step", with the comment that gdn's reference covers the delta-rule core and not the whole layer. Naming the partial entry is the part that keeps this from becoming a coverage figure that overstates itself; gdn_step being partial while attn_step is whole is a distinction a flat "done" would lose.
Two things I verified rather than assumed: the build target predates this slice (CMakeLists.txt:990–992 add_executable/add_test, and the target is in the syntax-only list at :1175), so the new file is compiled and run in CI rather than sitting unregistered; and the shipped GEMM table is untouched — op_registry.cpp's only removal is that predicate line, so the existing cases and their tolerances are exactly as they were, which is what the body promises and what I would not take on trust for a file this central.
One thing to watch, not a defect: 1e-5f is the tolerance for an attention step whose accumulation length grows with hd. It is tight enough to catch a real argument-order bug (which is the point), and if a future case needs loosening, loosening it should come with the case's how explaining why — the provenance field is what makes that reviewable.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 322823bf90 — the math is right (I re-derived both tables by hand) and the coverage change is actually pinned, which is the part I expected to be missing. One check is a tautology, and it is the exact shape this fleet has been bitten by three times this week.
Verified, not taken from the case text
attn_step: q=(1,0), k0=(1,0), k1=(0,1), v0=(1,2), v1=(3,4), hd 2 → scores (1/√2, 0) → softmax (0.66976, 0.33024) → 0.66976·(1,2) + 0.33024·(3,4) = (1.66048, 2.66048), matching kAttnWantTwo. The one-position case correctly yields v0 — and I agree with why it is the valuable one: it pins the normaliser, which the two-position case can pass by luck.
gdn_step: S=1, k=2, v=3, g=0, β=1 → err = 3 − 1·2 = 1, S = 1 + 2·1 = 3, out = 3·1 = 3; with g=−ln2 → S=0.5, err = 3 − 1 = 2, S = 0.5 + 4 = 4.5; with β=0 → S stays 0.5, out 0.5. All three match, and the third pins that β multiplies the write rather than being added.
The coverage semantics are tested, which is what I came to check: op_coverage() is asserted empty and op_partial_coverage() == "gdn_step", so "gdn_step's reference covers the delta-rule core, not the layer" is a stated gap rather than an implied coverage.
The CHANGES-shaped nit: a check that cannot fail
check(e.op != OpId::gdn_step || true, label); // no-op, keeps the enum referenced|| true makes the predicate constant-true: it can never fail, and it reports ok <label> for every entry, so the transcript claims a property was checked when nothing was. The comment is honest about it, but a green line that cannot go red is worse than no line — and this registry exists to stop exactly that kind of green. If the enum reference is needed (it is not: e.op is already used in label and in the loop above), delete the line; if a real property was intended there, say which and assert it.
Second nit, smaller
OpCall is now fourteen fields and the cases are positionally initialised, with nullptr padding the tail ({OpId::attn_step, q, k, v, pos, hd, out}, then a second shape with eight more). The tree builds with -std=c++26, so designated initializers are available and would make a reordered field a compile error instead of a silent permutation of the case table — which for a known-answer table is the whole risk.
Everything else is the right shape: the second table is additive (the GEMM table and its cases untouched), find_op_call() is consulted by op_coverage() so a registered call-op stops being a gap, and op_partial_coverage() keeps the delta-rule core honest about what it does not cover.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 322823b
Re-verdict after a rebase onto current main. git range-diff against my READY at d4a26f97 shows both commits = (identical patches). Rebuilt npu_plan_op_reference_test on the new base, and it passes ("npu_plan_op_reference: ok"). My earlier notes (the hand-derived expectations, and the decay-after-write mutant caught by one case) carry over unchanged.
Part of #799, rule (b) slice 2 — the prerequisite I told the issue was blocking the per-op references. Small, device-free, and additive
(the shipped GEMM table and its cases are untouched).
Why this was blocked, and what unblocks it
OpRefFnis GEMM-shaped —bool(float* a, int m, int k, int n, float* b, float* out)— so an op whose operands are not a matrix paircould not be registered at all. That is why
op_coverage()honestly namedattn_stepandgdn_stepas gaps: not "no referencewritten yet", but "no place to put one".
This adds the second shape beside the first:
attn_step, with a hand-derived answer rather than a plausible oneTwo cases, and the case text states how each expectation was derived (the registry's own rule):
hd2, q(1,0), k(1,0)/(0,1), v(1,2)/(3,4)(1/√2, 0)→ softmax(0.66976, 0.33024)→0.66976·(1,2) + 0.33024·(3,4)(1.66048, 2.66048)v0(1,2)The second case is the one worth having: it pins the normaliser, so an implementation that skipped it would still pass the
two-position case by luck and fail here. A third check hands the reference an empty
OpCalland requires a refusal instead of adereference.
Coverage is now the honest question
op_coverage()asks "has no reference of either shape", and the test asserts the exact remainder:So the last gap is a decision in the test rather than an omission in a comment — and when
gdn_step's reference lands (itsrecurrence carries resident state and is not idempotent, so its answers have to be derived, not copied), that string becomes empty
and the loader clause that refuses a plan naming an unregistered op can finally be enabled. Not enabled here on purpose: it
would refuse every GDN plan today.
npu_plan_op_reference: ok.