Repository navigation
npu: mtp::run's one-pass round for a head that does not read the hidden after t1 (refs #1781) - #2253
Conversation
…en after t1 (refs #1781) A DFlash draft drafts from the base's committed rows plus the last token's embedding, so it does not need the base's hidden after t1. Such a head returns false from DraftHead::needs_last_hidden(), and run() then verifies [t1, d1..dk] as one batch of k + 1 rows instead of a step plus a k-row batch: draft i is checked against row i, the round commits t1 plus the accepted drafts, and the drafter's commit_rows() gets those rows. k is held to max_batch_rows() - 1. The default (true) keeps the two-pass round the MTP head needs. tests/mtp_draft_test.cpp [17]: the text, recurrent state and positions equal plain decode's for perfect, garbage and half-right heads at k = 1..12, both documented row-count shapes; a fully accepted chain takes ceil(N / (k + 1)) batches and no step (k = 7: 0.125 base passes per token against 0.250); every committed row reaches the drafter once, in order; a consumer stop and a one-row batch. Seven mutants each fail it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ready for review at b14db85 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at b14db85: READY
It merges cleanly onto main. tools/copyright.py --check: rc 0. CI build at b14db85 is green: 100% tests passed, 0 tests failed out of 262.
Code (mtp::run(), the !needs_last_hidden() branch): [t1, d1..dk] is verified as one batch, draft i is checked against row i, commit_verify(take + 1) commits t1 plus the kept drafts, commit_rows gets kept = {t1, d[0..take)}, and the next t1 comes from row take. room = min(k, max_batch_rows() - 1, left), and with nd <= 0 the plain-step() path still commits t1's row to the drafter. The default needs_last_hidden() = true leaves every existing head on its two-pass round.
Checked at this head (review build, toy base, no device):
mtp_draft_test:all checks passed.- A mutant not in the body's table:
roomwithoutleft, sokis not clamped to the tokens the run may still emit. rc 1, 2 FAIL, startingone-pass k=4 head=perfect rows=prefix: text, state and positions equal plain decode. The last round would run pastmax_tokens. After restoring the file the tree is clean and the test passes again. - The body's "ignore
needs_last_hidden()(always two-pass)" mutant is also the right negative control: it shows the one-pass path is the one taken, not just that the outputs agree.
Not re-run here: the body's host-GEMM probe on the real forward (Qwen3-0.6B and the fabricated qwen3/qwen3_5 containers, bit-identical next-step logits). The probe is not committed. The toy-base test covers the bookkeeping, and commit_full_accept_test (#2243/#2247) already covers the forward's own full-accept commit.
|
On the two asks at b14db85:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- the round is right, the test is the right shape, and one contract needs a sentence because the header passes more rows than it commits
The one-pass round itself checks out line by line. t1 becomes row 0 of the verify batch, left = max_tokens - tokens.size() bounds the drafts the run may still emit, room = min(k, max_batch_rows() - 1, left) keeps the batch inside the base's row budget, nd is clamped twice (to room and to d.size()), the commit is take + 1 rows where take is what the consumer actually kept, pos += take + 1, and the next round's t1 is the choice in row take. The degenerate paths are handled rather than assumed: nd <= 0 consumes t1 the plain way so the base has run every token it was handed, and a == 0 commits exactly t1.
The test is the shape I would ask for, and two of its arms are the ones that actually make this falsifiable:
- token equality against plain decode for perfect, garbage and prefix heads -- the only check that proves a speculative loop is a fast loop and not a different loop;
m.bad_pos() == 0 && m.consumed() == P.size() + r.tokens.size()-- every position handed to the base is consistent and every emitted token was consumed, which is where off-by-one errors in this class of code live;passes_per_token() < 0.40with a perfect k=8 head, and> 1.0with a garbage head -- the round's purpose stated as a measurement on both sides, so "it is faster" is a number and "it was not worth it" is also a number. That pair is worth more than the rest of the arms together, because a speculative loop can be correct and still be a pessimisation.
One ask, and it is about the contract rather than the code. The header passes the full hidden-row vector with a smaller count:
head->commit_rows(kept, rows, take + 1); // `rows` is base.last_hidden_rows(), length nd + 1and the input check deliberately tolerates either shape (got != take + 1 && got != nd + 1). That is safe only if every head reads count rows and not rows.size() -- a head that iterated the vector would take the hidden states of drafts that were not committed, i.e. of positions the base will recompute differently next round. Since commit_rows(drafts, rows, count) is the interface a DFlash head has to implement, say it at the declaration: read count rows; the vector may be longer, and the rows past count describe uncommitted positions. One sentence, and the tolerance stops being something a reader has to reverse-engineer.
Everything else matches the sibling #2245 in shape and quality: needs_last_hidden() defaulting to true means every existing head keeps its two-pass round, so this cannot silently change the MTP head that docs/npu.md says needs it -- the new path is opt-in by construction, which is the right default for a change like this.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b14db85
Built and ran it: all checks passed, 252 PASS lines -- and the arm set is the "equivalent output, fewer passes" shape this kind of change needs, at scale.
The equivalence arms are the safety case, and they are stated as three head qualities crossed with the pass count:
k=0 head=perfect / head=garbage / head=half: tokens identical to plain decode
final state == prompt+emitted (28), no skipped positions
draft rows never exceeded max_batch_rows (over=0)
head=garbage is the arm I look for first. A one-pass round that derives t1 differently can pass on a perfect head and drift on a bad one, because a perfect head accepts every draft and never
exercises the fallback path -- so "identical to plain decode" only means something when the head is wrong in each of the ways a real one is.
And the three invariants per arm are the ones a loop can break silently: tokens identical (behaviour), final state equals prompt + emitted with no skipped positions (the KV/state accounting -- a
skipped position is invisible in the text and fatal two rounds later), and over=0 draft rows (the batch bound the head's max_batch_rows() sets).
The one-pass claim is measured against its alternative rather than asserted: "one-pass k=7: same text as the two-pass round in fewer base passes" -- same output, fewer passes, which is the entire
premise. And one-pass k=12 head=half rows=batch: never past 8 rows (over=0) + the drafter saw 24 rows in order (want 24), no hidden pins the interaction between the wide draft and a narrow batch:
the drafter sees every row in order even though the batch is capped at 8.
The two edge arms are the ones I would have asked for:
- "on_token false on the 6th token stops the run with 6 tokens emitted and committed" -- a stop signal must not leave the count and the committed state disagreeing, and whichever of the two it
reported would be wrong if they diverged; - "one-pass with max_batch_rows() == 1: plain steps, no batch, the drafter still sees every row" -- the degenerate configuration, where the optimisation has nothing to do, still routes every row to the
drafter.
Scope is stated the right way again: "Refs #1781 … the host half of 'the loop inside forward-serve'" -- which part, and that it is half.
One note: 252 PASS lines is a lot to read on a failure, and the harness prints them all. A failing run would be findable by grep FAIL, but a summary line at the end ("all checks passed" is
there -- "N of N, M arms") would make the count checkable at a glance, the way spec_greedy prints ok (12 checks).
Not run: the loop against a real head (npu-hold is off; this is the host half).
Refs #1781 (the speculative loop on the NPU, the host half of "the loop inside forward-serve").
What
mtp::run()is the engine's draft/verify/commit loop (npu/mtp/draft.h). Each round it runst1(the base's own token) throughstep(), so the head can read the hidden aftert1, and then verifies the drafts in a batch: two base passes per round. The MTP head needs that, anddocs/npu.mdrecords that folding the step into the batch fails for the MTP head.A DFlash draft (#1780) drafts from the base's committed rows plus the last token's embedding, so it never reads the hidden after
t1. This addsDraftHead::needs_last_hidden()(defaulttrue, so every existing head keeps its round). A head that returnsfalsegets the one-pass round:[t1, d1..dk]is verified as one batch ofk + 1rows; draftiis checked against rowi;t1plus the accepted drafts (take + 1rows), and the drafter'scommit_rows()gets exactly those rows,t1's first;t1is the base's choice in rowtake;kis held tomax_batch_rows() - 1(t1takes a row) and to the tokens the run may still emit;t1is consumed by a plainstep(), so the base has always run every emitted token.This is the round that
docs/oga-drafter-path.md's "one pass (verify only)" rows price, and thattools/dflash2_proberan by hand on #1781.Proof
tests/mtp_draft_test.cpp[17] (toy base, no device; in CI throughmtp_draft):k= 1, 2, 4, 7 and 12 (12 is clamped to 7 on an 8-row base), with the base returning either the committed prefix or the whole batch inlast_hidden_rows();ceil(24 / (k + 1))batches and no step. Atk = 7that is 0.125 base passes per token, against 0.250 for the same head through the two-pass round;The whole test passes (252 checks). Seven mutants each fail it:
takerows instead oftake + 1iagainst rowi + 1t1's row fromcommit_rowsneeds_last_hidden()(always two-pass)kuse allmax_batch_rows()rowst1from row 0commit_rowson the plain-step pathThe real forward, host-GEMM (a scratch probe, not committed): a
BaseModeloverQ4nxNpuForwardwith 8-row verify batches and an oracle head built from a plain greedy run. Tested on the two-layer qwen3 and the qwen3_5 (GDN + attention) containers thattests/batch_check_state_ci.pyfabricates, and onQwen3-0.6B:k = 3and7, 24 tokens each;k = 7perfect takes 3 batches and 0 steps.The full ctest of an
ONEBIT_NPUbuild with the device tests excluded: 265 of 265, 0 failed.Not in this PR: the DFlash drafter as a
DraftHead, and serving the loop fromforward-serve. Both are #1781 work that needs device runs.🤖 Generated with Claude Code