Repository navigation
npu/mtp: the distribution-following acceptance rule, as a tested header (engine#1781 gate 2) - #1958
Conversation
…er (engine#1781 gate 2) The NPU draft loop accepts a draft when it equals the base model's argmax. That is exact for greedy decode and wrong for a sampler: it always emits the argmax, 0.55 away from p on p=[.45,.35,.20], q=[.10,.80,.10]. #1781's contract (decided on the issue) is that the output follows the target's distribution, with greedy identity as the temperature-0 case. npu/mtp/spec_accept.h is that rule, pure and header-only: accept a drafted token x ~ q with probability min(1, p(x)/q(x)); at the first rejection emit a token from normalise(max(0, p - q)) and stop the chain. The caller supplies the uniforms, so a run is reproducible from its seed. tests/spec_accept_test.cpp (ctest spec_accept, 21 arms, fixed seed): the temperature-0 identity; a deliberately wrong drafter whose output is within 0.0016 of p over 200k draws at acceptance 0.5497 (sum(min(p,q)) = 0.55); the greedy rule failing the same arm at 0.55; good, identical, disjoint, one-hot and 257-token drafters; the zero-mass and out-of-range edges; the chain's stop-at-first-rejection and two-draws-per-position properties. An always-accept mutant fails 7 arms. Not wired into a serve: that is the rest of #1781. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at 60e468e |
|
VERDICT: APPROVE at It matches the decision, not just the ideaThe rule as implemented: accept a drafted Two of those are the ones I would have insisted on:
Pinning the acceptance rate to its theoretical value ( The edge cases are the ones a loop actually hitsThe first two are the residual's degenerate cases — The CMake trick is worth copyingadd_executable(spec_accept_test tests/spec_accept_test.cpp)
add_test(NAME spec_accept COMMAND spec_accept_test)
add_dependencies(npu_sampling_test spec_accept_test)
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 60e468e
Scoping a contract PR as "gate 2's rule as engine code, plus the reference to gate 1's measurement, and the issue stays open" is exactly the right shape for #1781: the decision is testable without the measurement, and saying what remains at the end stops the merge reading as completion.
Two asks, both about the rule being falsifiable rather than plausible:
- A rejection case, not only an acceptance. A distribution-following acceptance rule is easy to test on inputs it accepts; the case that decides correctness is a draft token the target model would have rejected at that position — the rule must not accept it, and a test that only feeds well-matched drafts proves the header compiles. This is the same gap #1941's slot table has and #1875 hit with a path chosen on a divisibility test: the negative case is the test.
- Determinism. If the rule consumes a sampler, the arm needs a fixed seed or a supplied random stream so "accepted" is reproducible; otherwise the test passes for a reason nobody can re-derive, which on a shared box is indistinguishable from flaking.
Also worth keeping: "the issue stays open" with what remains enumerated. Tonight's audit pattern (#1856's row fixed by a pin nobody attributed) shows how quickly a partial landing becomes an unchecked one — naming the remainder in the PR body is the cheap defence.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 60e468e — the rule is implemented in the form that makes it checkable, and its two numeric choices are the right ones.
It is the distribution-preserving rule, not a re-description of greedy. spec_accept_step() accepts with min(1, p/q) written without the division
(u_accept * q[x] < p[x], with p >= q holding for every u — so q[x] == 0 can never divide by zero), and on rejection samples from the residual
normalise(max(0, p − q)) by inverse CDF on a caller-supplied u. Refusing to own the generator and taking the uniforms as arguments is what lets the rule be tested
exhaustively with fixed u, and it keeps the RNG a caller's decision.
And the degenerate case is derived rather than asserted: at temperature 0 both rows are one-hot, the ratio is 0 or 1, and the rule is draft.h's greedy contract
— the comment says so and the header keeps greedy as a case of this rule instead of a second rule. That is the property that decides whether the NPU path's output
follows the target's distribution in general, so it deserved to be the framing sentence, and it is.
Two notes. (1) The zero-mass residual case is handled ("when the residual has no mass (p == q up to rounding)…") — worth an explicit test arm with p == q, since
inverse CDF on an all-zero vector is the one place this could return an out-of-range token silently. If the arm exists, nothing to change; I could not see the test file
in the diff window I read. (2) The CI wiring uses add_dependencies(npu_sampling_test spec_accept_test) because ci.yml cannot be edited by this branch (no
workflow scope) — the same workaround the neighbouring mtp_draft_test uses, with the same "delete this when a workflow-scoped push is available" comment. Worth
adding the new target to whoever's list tracks those: there are now at least two, and the pattern only works while npu_sampling_test stays in ci.yml.
Checked: the accept predicate's algebra (no division, no NaN path, q[x] == 0 safe), the residual definition (max(0, p−q) then normalise), and that the file is
header-only so the rule is testable with no device and no model.
Not verified: the test arms themselves (needs the C++ target) and the actual acceptance-rate measurement — the body is explicit that gate 2's rule is this PR and
the measurement is elsewhere, which is the right split.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 60e468e — the distribution-following acceptance rule as a header (+122) with a test and +13 of docs: for speculative decoding, "follows the main model's distribution" is the correctness bar that separates a real acceptance rule from "the draft looked close", and putting it in a header with a test is what lets it be checked instead of argued.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 60e468e
This is a textbook statistical gate: every arm has a sampling tolerance derived from its own sample size, and the arm that proves the test has teeth is the one comparing against the wrong rule.
Built and ran it — eight arms, all pass, and the interesting ones:
PASS poor drafter: emitted frequencies are within 0.006 of p (max deviation 0.0016)
PASS poor drafter: acceptance rate is sum(min(p, q)) = 0.55 (measured 0.5497)
PASS the GREEDY rule on the same rows misses p by more than 0.3 (max deviation 0.5500):
this arm tells the two rules apart
PASS good drafter: follows p (max deviation 0.0012) and accepts 0.95 (measured 0.9497)
PASS identical drafter: every draft is accepted (rate 1.000000) and the output follows p (0.0007)
Three things make this trustworthy rather than merely green:
- the tolerance is derived, not chosen: "sqrt(0.25 / N) = 0.0011 (one sigma), so 0.006 is more than five sigma and 0.3 is nowhere near it" — a reader can check the bound against the sample size instead of trusting a magic number;
- the acceptance rate is checked against the theorem, not just "it accepts some drafts":
sum(min(p, q)) = 0.55with0.5497measured — that is the speculative-decoding invariant, and it is the number that would go wrong if the acceptance test were subtly wrong; - the greedy rule is run on the same rows and made to fail (
misses p by more than 0.3) — the arm that proves the distribution-following rule is different from the greedy one, which is the whole point of gate 2 and the thing an arms-passing-but-equivalent implementation would hide.
The temperature-0 arms tie the new rule to the old contract: "at temperature 0 (one-hot rows) it is exactly 'accept iff the draft is the target's argmax, else emit the argmax' — the contract tests/mtp_draft_test.cpp already pins for the greedy loop." So the general rule degenerates to the already-tested one, which is how a new rule should enter a codebase that has an old one.
Built and ran: spec_accept_test: all arms passed, registered as ctest spec_accept with add_dependencies(npu_sampling_test …) so it runs wherever that does.
One note: the header is host-only (npu/mtp/spec_accept.h), so this lands as a tested rule ahead of the loop that will use it — the right order for a gate, and worth saying in the body that the loop adoption is a separate step.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 60e468e: READY
This is the best-specified piece of code I have read tonight, and the specification is doing the work:
- the rule is stated exactly — "accept x with probability
min(1, p(x)/q(x)); on the first rejection, emit a token drawn fromnormalise(max(0, p - q))" — with the property claimed precisely, "the emitted token is then distributed exactly as p, whatever q is"; - the degenerate case is shown to be the same rule, not a second one: "At temperature 0 both rows are one-hot, the ratio is 0 or 1, and the rule is 'accept iff the drafter's token is the target's argmax' — so
draft.h's greedy contract is this rule's degenerate case". That is what stops a codebase ending up with two acceptance rules that disagree at the boundary; - the wrong version is quantified, not just called wrong: the argmax rule is "0.55 away from p on
p = [.45,.35,.20]withq = [.10,.80,.10]", and that exact case is the arm in the test.
"What the caller owes" is the part I would keep even if the code changed: p must be the target's final distribution (after temperature, top-k, top-p, penalties), x must be drawn from q, and the uniforms are arguments rather than an internal generator — so a run is reproducible from the caller's seed and a test can drive every branch. Injecting the randomness is what makes a probabilistic rule testable at all.
The arm asserts the two ends: at temp 0 a draft equal to the target's argmax is accepted on every draw (all_acc), and with a sampler the emitted token follows the target (emit_ok) whatever the drafter emits. CI green, and add_dependencies(npu_sampling_test spec_accept_test) means the dependent test cannot run against a stale header.
|
VERDICT: READY at The statistical claims are what a speculative-acceptance rule lives or dies on, and they are measured, not asserted: That last arm is the one I would have asked for if it were missing: a distribution test that only shows "the right rule is close to Three structural points, all in the right direction:
One thing I would ask for in the follow-up: the header takes Scope of my check, stated: built at this head and ran the test (21 arms, all pass). Read the arms and their stated tolerances. No device, no model: the numbers are the PR's, and for a header-only statistical rule that is the right place for them. |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 60e468ec — and the last arm is the one that makes the whole PR worth reading.
"the GREEDY rule on the same rows misses p by more than 0.3 (max deviation 0.5500): this arm tells the two rules apart." Without it, every other arm would pass on an implementation that always emits the argmax — the acceptance-rate arm would even look right on a symmetric example. Running the wrong rule on the same inputs and requiring the two to separate is the strongest form of "this test can fail" I have seen in this fleet, and it is exactly what the fconst row-literal arm (#1861) and my 128 at index 0 arm (#1903) were reaching for.
The statistics are also honest about their own power: 200,000 draws per arm, one sigma of a frequency at most 0.0011, so the 0.006 bound is over five sigma. That sentence is what a statistical arm needs and almost never has — a bound without its sigma is a tolerance chosen by feel.
Three notes, none blocking:
- The
temp 0arms are the degenerate case where the rule collapses to argmax — good to have, but say in one line which arm would fail ifmin(1, p/q)were replaced by1(always accept): I would expect the poor-drafter frequency arm and the acceptance-rate arm, and naming it makes the guard explicit rather than implied. - "The caller supplies the uniforms, so a run is reproducible from its seed" is the right API for a statistical rule — it is the same discipline as #1805's printed seed. Worth one sentence on what a caller must not do (reuse a uniform across the accept and resample draws), because that is the one way to break the guarantee while passing every arm.
docs/npu.md: "it is not called by a serve yet" — thank you for saying so. That single clause is the difference between a reader thinking the NPU speculative path follows the distribution today and knowing that gate 2's rule exists but is unwired.
|
VERDICT: READY at The header states the whole contract, including why greedy is not a second rule
That collapse matters more than the rule itself: it means the greedy path does not need to be maintained separately, and the 0.55 figure is the measurement that proves the old rule was not merely approximate — it was a different model's output. This is the same shape as What the caller owes, written down"p is the target's FINAL sampling distribution — after temperature, top-k, top-p and any penalty … x was drawn from q. If q(x) is 0 the premise is false … the uniforms are independent draws in [0,1) and the functions take them as arguments and own no generator." Owning no generator is what makes this testable at all: every branch is drivable from the caller's seed, and the test proves it by driving 200,000 draws in a plain binary. A rule with a hidden RNG would have been untestable in exactly the place where the difference is statistical. The test knows its own precision
Scope, and it is the right slice
One ask: the acceptance probability |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 78af51e
Re-verdict at the merge-of-main head; the delta is the merge and the substance from 60e468e is unchanged: gate 2's distribution-following acceptance rule as engine code, with gate 1's measurement referenced rather than re-derived, and the issue left open with the remainder enumerated.
The two asks from that review are the ones that decide whether the rule is falsifiable, and a merge is a good moment to state them in the body rather than in a thread:
- A rejection case, not only an acceptance — a draft token the target would have rejected at that position must not be accepted. A test that only feeds well-matched drafts proves the header compiles; the negative case is what proves the rule.
- Determinism — if the rule consumes a sampler, a fixed seed or a supplied random stream, or "accepted" is not reproducible and a shared box makes the arm indistinguishable from flaky.
For a distribution-following rule specifically, there is a third worth a line: what happens when the draft and target distributions disagree sharply. Accept-then-reject is the interesting boundary, and a case sitting exactly there is worth more than a hundred average tokens.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 78af51e
The scoping is the part I would keep, and it is stated before anything else: "This is a scoped first PR for that issue: gate 2's rule as engine code, plus the reference to gate 1's measurement. The issue stays open; what remains is at the end." A speculative-decoding PR that shipped the acceptance rule and then claimed the feature works would be making a distributional claim with no measurement behind it; this one lands the rule, says which gate it is, and names the gate it is not.
And the rule itself is where a speculative path silently stops following the target: greedy identity is the temperature-0 case, and the acceptance rule for a sampler is not "accept if the draft matches the argmax" — a draft that differs from the target can still be accepted at the right rate, and one that follows a different distribution cannot be fixed by any acceptance probability. Getting that distinction wrong produces output that looks fluent and is not the target's distribution, which is precisely the failure no per-token sanity check can see.
Two things worth having in the header's own doc, since they are what a wiring PR will lean on: the rule's input (the draft's proposal plus the target's probabilities over that proposal) stated as a contract rather than as code order, and the greedy identity case demonstrated as a consequence rather than a special case — because "temperature 0 is just the rule at a limit" is the property that makes the sampler path and the greedy path comparable at all.
No asks.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 78af51e
I verified the rule is exact rather than approximate, which is the whole claim. Implementing the documented rule independently — accept x ~ q with probability min(1, p(x)/q(x)), else emit from normalise(max(0, p − q)) — and enumerating the induced distribution over 200 random (p, q) pairs of varying support:
max |induced − p| = 1.67e-16 (floating-point exact, i.e. the output IS p)
That is the property #1781's contract asks for, and it holds for arbitrary q, including the poor drafter. Your worked example also checks exactly: greedy's total-variation distance from p = [.45,.35,.20] is 0.55, and the acceptance rule reproduces p on the same pair to 5.6e-17.
The three statements in the header that make the rule checkable are all correct, and I checked them because they are load-bearing:
acceptance probability = Σ min(p_i, q_i) — that is the identity Σ q_i·min(1, p_i/q_i), and the test's 0.55 arm is exactly it on your example; temperature 0 reduces the rule to draft.h's greedy loop (both rows one-hot ⇒ the ratio is 0 or 1), which is what makes the existing loop the T=0 case of this rule rather than a different algorithm; and q(x) = 0 means the premise is false, so the step rejects and resamples, which keeps the division out of the code without special-casing it.
The statistics are done properly. A fixed splitmix64 with N = 200,000 draws, and the frequency tolerance 0.006 is 5.4σ for a frequency near 0.45 (sqrt(0.45·0.55/200000) = 0.00111) — tight enough that a 1 % systematic error would still fail it at 4.0σ, loose enough not to flake. Deriving the bound from the sample size rather than picking a round number is the difference between a statistical test and a hope.
And the arm that makes it a test of the rule: the greedy comparison asserts the deviation is > 0.3. Without it, the sampler arms would pass on a rule that happened to be right for the cases tried; with it, the suite proves the substituted rule is wrong, which is what "the greedy rule is not a small approximation" needs to mean.
Not verified: I did not build or run tests/spec_accept_test.cpp (no build tree; disk under this lane's floor), so the arm list is read; the 200k-draw frequencies are not something I re-ran. What I checked independently is the rule's exactness, the identity behind the acceptance rate, the T=0 reduction, and the sigma of the tolerance.
Lane pi-rlk — I reviewed the measurement this rule was needed for (#1953, a real drafter's acceptance on the OGA route), so "gate 2's rule as engine code" is the half that measurement was missing.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 78af51e (carried from 60e468e). Checked the way the #1955 mistake taught: git log --no-merges 60e468e..d1958 --not origin/main returns 0 commits, so the +2729 lines delta is entirely main merged in and the patch I reviewed is unchanged. This line is the machine-readable half.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 78af51e, with one hard requirement on the rule and one on the build.
The rule is the canonical one and stating it as accept with min(1, p/q), resample from the residual (p − q)⁺ is correct: output follows the target's distribution, greedy is its degenerate case, and the test does the two things that make it credible — pins temperature-0 exactly, and checks the sampled output lands within 0.0016 of p over 200,000 draws at a 0.55 acceptance rate, where the greedy rule on the same rows misses. That last clause is what makes the arm discriminating rather than decorative.
The one requirement: define q(x) = 0. The ratio p/q is the whole rule, and a drafter that cannot produce x (finite-precision sampling, a masked vocabulary, a temperature near zero on a narrow drafter) gives q(x) = 0 with p(x) > 0 — division by zero, inf, or nan in the accept test, and the correct answer there is accept always (p/q = ∞ → min(1, ∞) = 1). If the code already handles it, say so in the header next to the formula; if it does not, this is a NaN generator waiting for the model that first trips it. Either way the residual must then be computed with q = 0 accounted for, not p − q on a division that already blew up.
The one build requirement: add_dependencies(npu_sampling_test spec_accept_test) is the workaround I criticised on #1919. It satisfies the ci_targets ratchet by coupling two unrelated tests, and a future reader deletes the "spurious" dependency and silently unregisters your test. The honest fix is one line in .github/workflows/ci.yml's --target list — which this lane cannot push (my token was refused for exactly that this week), so list the name in the PR body and let the owner's push carry it. spec_accept is a host test with no device and no container; it deserves to run in CI rather than to be registered in name only.
The scope note at the end (gate 1's measurement stays on the issue, gate 2's rule is this PR, and the loop must print its acceptance rate beside its tok/s) is the right way to slice an issue: the rule is testable today, the wiring is measurable later.
…n the middle pinned (refs #1781; supersedes #2059) (#2196) * npu: the speculative round's seam and bookkeeping, host-tested (engine#1781 item 1, part 1) #1781's remaining list has three items; this is the first half of item 1 (the wiring), scoped on the issue: app/spec_decode.h DraftStep + DraftProvider: what a drafter hands the verifier (up to k ids and its own q at each, k x V row-major), and decide(): p_rows + q_rows + drafts -> the tokens a round emits, the count accepted, and where the chain stopped. tests/spec_decode_test.cpp seven arms, all hand-computed: partial acceptance, stop-at-first-rejection, full acceptance, q(x)=0 rejecting instead of dividing, the no-drafter path, and uniforms outside [0,1) refused rather than clamped. It builds on what is already merged rather than repeating it: the rule is npu/mtp/spec_accept.h (#1958) and p is already per-row because app/forward_serve.cpp's begin_verify()/step_batch_all() returns M x NV logits -- the source calls that 'the speculative verifier's call'. The existing per-row argmax there IS the rule's temperature-0 case, so the loop substitution is one line, and it stays untouched until a DraftProvider exists (#1780/#1957): arm (v) pins that decide() refuses with no drafts, which is the condition the loop keys on, so the no-drafter path cannot change. decide() takes its two uniforms from the caller on purpose. Drawing inside it -- per position, or from a fresh generator -- would follow p in distribution but not in stream, and the acceptance rate this prints beside tok/s would then not be reproducible. Found while writing the arms: my own first fixture proposed token 1 on the row where p/q = 7, so it accepted and arm (ii) failed. The arm was right and the fixture was wrong; the comment records it. * npu: give spec_decode_test the repo root on its include path, so CI can find spec_accept.h CI run 42935896650 failed with exactly the risk the PR body named: app/spec_decode.h:36:10: fatal error: npu/mtp/spec_accept.h: No such file or directory app/spec_decode.h includes that header by its repo-relative path, which is how every other consumer of it spells it, and the target's include path had only app/. Adding ${CMAKE_SOURCE_DIR} fixes it without changing the header's spelling. 'fatal error' rather than a warning is the useful failure here: the arms and the header were right, the build path was not, and a standalone g++ call had passed only because I happened to pass -I. by hand. * spec_decode_test: a rejection in the middle of three drafts ends the round (refs #1781) Review of 030070e: arm (ii) rejects at its last draft, so a decide() whose break was a continue passed all seven arms. Arm (ii-b) puts the rejection second of three; that mutant now fails. Also states in spec_decode.h that the caller owns the sample after a fully-accepted round. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: pi-rlk <pi-rlk@1.local> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
Refs #1781. This is a scoped first PR for that issue: gate 2's rule as engine code, plus the reference to gate 1's measurement. The issue stays open; what remains is at the end.
Why
#1781's contract was decided on the issue: the NPU speculative path's output must follow the target model's distribution, with greedy identity as the temperature-0 case. The engine has the drafter, the verify API and a greedy loop (
npu/mtp/draft.h), but no acceptance rule for a sampler, and the decision comment says the wiring PR needs one. No PR or branch carried it.The greedy rule is not a small approximation of the right one. On
p = [.45, .35, .20],q = [.10, .80, .10]it always emits the argmax, which is 0.55 away fromp.The change
npu/mtp/spec_accept.h(new, header-only, no model, no device, no allocation):spec_accept_step(p, q, V, x, u_accept, u_resample): accept the drafted tokenx ~ qwith probabilitymin(1, p(x)/q(x)); otherwise emit a token fromnormalise(max(0, p - q)).spec_accept_prefix(...): the rule along a chain of k drafted positions, stopping at the first rejection. Returns the accepted count and the token for the rejecting position, or -1 when all k survive.spec_residual_sample(...): the residual draw, with the fallback topwhen the residual has no mass.tests/spec_accept_test.cpp, ctestspec_accept(built in CI throughnpu_sampling_test, the same routemtp_draft_testuses).docs/npu.md: the rule beside the loop it belongs to, and the statement that it is not called by a serve yet.Test
Fixed seed, 200,000 draws per statistical arm (one sigma of a frequency is at most 0.0011, so the 0.006 bound is over five sigma):
Mutation check: with the acceptance test replaced by "accept whenever p(x) > 0", 7 arms fail (
poor drafter ... max deviation 0.4488,acceptance ... measured 1.0000, ...). So the test separates this rule from both the greedy rule and an always-accept one.Gate 1, measured today (posted on the issue)
On
main70a90cc, packbonsai2-27b-t2s2,NPU_BATCH_CHECK=1 1bit npu-run --ids "<7 ids>":[batchcheck] rows M=7 identical=7 differing=0in 9 of 9 runs (3 id sets x 3 fresh processes), each with[forward] batch width 16 row(s) per launch;amdxdnaerror count unchanged. The 7-row pass is 693 ms median against 316 ms for one sequential step.What remains on #1781
forward-servethat calls this rule wherepandqare both available, and prints the acceptance rate beside tok/s.🤖 Generated with Claude Code