Repository navigation
feat(mtp): per-row positions in the MTP draft chain — the blocker #102 named - #103
Conversation
…102 named The MTP draft KV is a second, independent time base from the target's, and it was keyed on ONE absolute position in four places. All four now take the row's own value, so a group whose rows sit at different positions drafts in one batched MTP-block forward instead of B single-row ones. * `MtpHiddenCapture::store_rows(seqlen_offsets, ..)` tags a capture with one start per row (`CaptureOffsets`) rather than `seqlen_offsets.first()`. The vector already existed; it was being collapsed. * `propose_chain_batched` takes `chain_starts: &[usize]` and, when they differ, hands `starts[r] + i` to the new `MtpBlock::forward_step_rows` — a positions vector for RoPE and for `dsv4_attention`'s per-row `row_q0`. * `front_align_draft_group` left-aligns a ragged group's draft KVs where `batch_draft_caches` refused them, and `drop_dead_prefix` takes the padding back out before any length is recorded. * `draft_group_key` therefore drops the absolute chain start, restoring a partition bounded by `depth + 1` rather than by the batch size. Flag-off byte-identity is structural: `resolve_capture_offsets`, `resolve_ragged_chain_starts` and `resolve_draft_leads` all fold a uniform batch back to the pre-change scalar representation, which runs the old code verbatim down to RoPE's one-element fast branch. `PerSequence` is still refused. The refusal moves to the batch admission layer — `engine/mod.rs`'s `CacheInstruction::In` gate and the scheduler's `cache_bucket_len` bucket key — both named by `batch_admission_admits_ragged_cohorts`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Metrics Report━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Language Files Lines Code Comments Blanks ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ C Header 5 305 210 52 43 CSS 2 1181 1036 34 111 CUDA 72 24328 17592 4018 2718 Dockerfile 1 39 22 8 9 JavaScript 16 3546 2676 482 388 Jinja2 7 694 656 5 33 JSON 74 4600 4597 0 3 Makefile 1 6 5 0 1 Metal Shading Lan| 33 12224 9431 1142 1651 PowerShell 1 300 227 30 43 Python 143 14830 12217 797 1816 Shell 23 5756 3964 1412 380 Plain Text 4 3801 0 2479 1322 TOML 33 1485 1292 43 150 YAML 3 25 23 2 0 ───────────────────────────────────────────────────────────────────────────────── HTML 4 2687 2604 43 40 |- CSS 2 543 479 37 27 |- JavaScript 1 1233 1215 12 6 (Total) 4463 4298 92 73 ───────────────────────────────────────────────────────────────────────────────── Jupyter Notebooks 4 122 83 23 16 |- Markdown 1 60 30 22 8 |- Python 1 122 113 1 8 (Total) 304 226 46 32 ───────────────────────────────────────────────────────────────────────────────── Markdown 190 37974 0 29143 8831 |- BASH 71 1632 1192 324 116 |- C 2 12 12 0 0 |- CUDA 2 84 56 16 12 |- JSON 18 708 708 0 0 |- PowerShell 1 1 1 0 0 |- Python 23 1008 787 113 108 |- Rust 65 2048 1713 77 258 |- TOML 6 207 164 0 43 |- YAML 4 38 33 5 0 (Total) 43712 4666 29678 9368 ───────────────────────────────────────────────────────────────────────────────── Rust 662 310844 269120 14195 27529 |- Markdown 477 23762 471 20479 2812 (Total) 334606 269591 34674 30341 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Total 1278 456205 332729 74994 48482 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
…ort it PR #93 landed `XsRollingCache::trim_tail_to(new_base: usize)` and `reconcile_xs_bases` on master while this branch was making `base`/`tokens` per-row `Vec<usize>`. The 7 resulting errors are semantic, not textual: `base` is the same identifier naming two different quantities. - On master `tail` is `tokens - base` wide, so `base` IS the physical left edge of the buffer. Divergent `base` at equal `tokens` gives physical widths 4 vs 132 and the `slice_set` mismatch #93 exists to fix, reachable on plain decode through the prefix cacher. - Here `tail` is `[B, W, hidden]` and end-anchored, with `base[i] >= tokens[i] - W`. `base` is a logical resume point decoupled from the buffer; `BatchSrc::of` sets `v_slack_dim: Some(1)` with `v_slack_at_front: true`, so the widths already agree. So the reconciliation is required on one path and a regression on the other. Trimming every row to `base_max` raises the shortest-reach row's rollback floor to the batch's -- a per-row quantity replaced by a batch-wide scalar, the same shape as the cohort min-rollback #92 removed and the padding traps #102/#103/#104 each caught. This is the last layer still holding one. - `reconcile_xs_bases` takes `xs_per_seq` and returns early when set. Flag off it runs verbatim; #93's two mutation-proven tests pass unchanged (they run flag-off), with only a private-field read swapped for the `resumable_from()` accessor, which is `base[0]` for a single-row cache. - `trim_tail_to` REFUSES a multi-row cache rather than reading `base[0]`. A scalar `new_base` cannot describe a trim of rows at different resume points, and proceeding returns a right-shaped tensor -- exactly what the caller checks -- so it names the reason instead (D18). `per_row_xs_bases_survive_a_batch_round_trip_and_are_not_flattened` is the test that discriminates: flag on, two rows at divergent `base` and equal `tokens` must batch AND each keep its own `base` through `split_row`. Both mutations were run rather than assumed -- trimming the tensors is caught by the width assertion, but reconciling only the logical `base` passes every shape check and fails solely on the per-row read. 8/8 mutations caught. One survivor on the first pass (`trim_tail_to` narrowing from the wrong end) was real: no test read tail CONTENT, because every fixture fed zeros. `trimming_the_retained_window_drops_the_oldest_rows_not_the_newest` feeds a ramp and closes it. 430 lib tests pass, 14 synthetic, workspace check clean, scoped clippy exits 0, zero new clippy diagnostics in the touched files, and rustfmt drift is 6 before and 6 after -- no upstream reformat churn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Admin-merged without review (no second reviewer). Bypass covered the review requirement only — the required Landed ahead of the rest of its stack because it is clear of the #122 interaction. #122 (ragged dense decode) landed on a master already carrying #100, and the two ragged-mask efforts now coexist in the same subsystem; "merges cleanly" is not "is coherent". This PR touches |
Stacked on #102 (
feat/mtp-step-per-seq-cachelen), which is stacked on #100 → #95 → #92. Review those first.The blocker, in one sentence
#92 named two blockers; #95 removed the first, #100 the second, #102 the third and named this one. This removes it.
cache_supports_per_sequence_advance,model_masks_ragged_batchesanddraft_chain_carries_per_sequence_positionsall pass for a V4 target now.🔑 The vector already existed, twice over
MtpHiddenCapture::store(seqlen_offsets.first().copied().unwrap_or(0), &xs)— the sameseqlen_offsets#100 discovered is already rowi's ownq0, collapsed to row 0 one line before it left the model. And a draft group's per-row chain starts arecache_lens[i] + u - 2, which #102's step already computes;draft_group_keywas flattening them into the partition key. Nothing is plumbed: noHashMap<seq_id, …>, no new field onModelInputs, no signature churn throughDecoderLayer. Same shape as #100'sseqlen_offsets[i]and #102's discardedlead_pad[i].The four sites, and how each closes
MtpHiddenCapture::storerecords row 0's offset for the whole capture, soextend_draft_kv_row'soffdescribes only the leading rowstore_rows(seqlen_offsets, ..)→CaptureOffsets, andextend_draft_kv_rowreadsoffsets.row(row)propose_chain_batchedtakes ONEstart_posand handsstart_pos + itoMtpBlock::forward_step, whoseposis both the RoPE position and the draft-KV slotchain_starts: &[usize]; when they differ,forward_step_rowspasses the vector asseqlen_offsets, which RoPE andragged_row_q0already know how to readbatch_draft_cachesrefuses rows whosecurrent_seq_lendiffers — under per-sequence advance, all of themfront_align_draft_group: truncate each row to its own chain start, shift its live run to END at the group's widest, concatenate.SingleCache::append's one shared offset is then correct for every rowu_i ≡ 1that is one group per sequencedraft_group_key(u, cache_len, carries_per_row)drops it; the partition returns toualone, i.e.<= depth + 1groups at any batch size🔑 Site 3 is where #102's defect-4 trap lives again, one cache down
Left-alignment is the only way a ragged group shares an append offset, but the length it leaves behind (
lead + live) is not the row's own. Recording that makes the next group pad relative to an already-padded length — the prefix compounds every step and the rows re-converge onto one length. #102's words for it in the target's cache: "the cohort barrier reappearing as padding."So
drop_dead_prefix(&mut row_cache, leads[slot])runs beforestate.filled = chain_starts[slot] + 1, andthe_draft_dead_prefix_does_not_survive_into_the_per_sequence_cachespins both halves: the stripped run's lengths stay{7+3k, 4+k, 6+2k}at k = 1/6/12, and the negative control (same loop, strip removed) not only inflates but has its spread collapse to a constant 2 — every row advancing at the fastest row's rate.lead == 0is a no-op that touches no tensor, so B=1 and every uniform group are structurally untouched.Why "flag off is byte-identical" is structural
Three dispatch functions, each returning the pre-change representation — not merely a vector that happens to hold equal values — for a uniform batch:
resolve_capture_offsets→ a one-elementCaptureOffsets, sorow(r)is literallyseqlen_offsets.first().copied().unwrap_or(0)for everyr;resolve_ragged_chain_starts→None, which runsMtpBlock::forward_stepwith onepos, a one-elementseqlen_offsets, RoPE'slen() == 1fast branch (not its per-row loop), andresolve_ragged_rowsfilteringrow_q0back toNone;resolve_draft_leads→None, which runsbatch_draft_caches+set_lenverbatim.No fifth flag.
resolve_draft_leadsis gated onper_seq_advance, coordinated withARC_MTP_PER_SEQ_KVandARC_V4_XS_PER_SEQexactly as #100 and #102 did it — and not decoratively:ragged_row_q0, the only thing that masks the padding this creates, is gated on the same variable.Somewith the flag off would left-align rows whose zero columns nothing masks, and a zero K row is not a masked row.draft_group_key's flag is load-bearing for the same reason in reverse: dropping the chain start under the old scalar path would not be slow, it would be silent — a row whose seed is not at the group's single chain start is dropped by the seed check, so it would stop drafting with no signal but a fallingtok_per_step(D18).🔴 A guard added, for a wrong answer that was reachable
Under
PerSequencethe step runs no cohort rollback, so the pipeline's resident batched cache stayscache_len + wwide while each sequence keeps its own shorter length. A later step that did not assemble the batch reads that stale width,window_okfails, and — becauselead_padis all-zero on such a step — #102's left-aligned guard cannot see it. Control would reach the deferral, which decodes against a cache whose write offset and whose RoPE positions no longer agree.!window_oknow also refuses wheneverkv_advance() == PerSequence, regardless ofrow_cache_lens.📊 Measured — CPU unit tests and plan arithmetic, not hardware (D14)
cargo test -p mistralrs-core --lib→ 434 passed, 0 failed (was 423 on #102's tip) ·--test synthetic_load_smoke→ 13 passed ·cargo check --workspace --testsgreen · scoped clippy lane green ·mistralrs-coreclippy diagnostics on the touched files diffed before/after: zero new · zero new rustfmt drift, checked like-for-like per file (mtp_pipeline0→0,deepseek430→30,lib.rs310→309,pipeline/mod.rs44→44).Mutation runs — 22 mutations, 21 caught
model_masks_ragged_batchesverbatim (the MTP block is a V4 decoder layer, so the samerow_q0masks its draft cohort), so it can never be the first failure inper_sequence_refusaland deleting it there is a true no-op. It is kept because the implication is a property of today's single-architecture discriminator, not of the two capabilities — andthe_draft_chain_check_is_implied_by_the_mask_check_todaynow asserts the implication over three cache shapes, so the day check (2) learns a second architecture, a test fails instead of the draft chain being silently inherited.The other 21 were all caught on the first run.
a_uniform_draft_group_never_reaches_the_ragged_chain_pathanda_uniform_draft_group_round_trips_through_the_batched_cache_unchangedexist because of #95/#98/#100/#102's warning: routing a uniform group down the ragged path is numerically invisible, so only a dispatch test can see it.drop_dead_prefix+state.filledlines cannot be reached without standing up a whole V4 pipeline.the_draft_dead_prefix_does_not_survive_into_the_per_sequence_cachessimulates that loop line-for-line against real tensors — it is a faithful mirror, not the loop itself, and it readscurrent_seq_len()back off the slot and the content back out of the tensor rather than recomputing either (which is exactly how #102's first draft of the equivalent test passed with the strip stubbed out).🔴 The honest gap —
PerSequenceis still refused, and the refusal moved AGAINThis does not move the B=128 number. With the draft chain per-row end to end, a fifth blocker is visible that none of #92, #95, #100 or #102 could see: the layer that admits a batch still assumes one batch has one length. Two sites, both above this file:
engine/mod.rsissuespre_op = CacheInstruction::Inonly when the scheduled completion id list changed. A stable cohort therefore takesNothingon every step after the first, andper_seq_advancerequiresassembled_here, which is set only on theInarm — so the mode would interleave with the cohort rule step by step rather than being on. feat(mtp): per-rowcache_lenin the fused step — the pipeline blocker #100 named #102's comment says such steps "take the cohort rule — correctly"; that is true in isolation and not true across steps, which is what the new!window_okguard above now refuses rather than serves.scheduler/default_scheduler.rspartitions the running set bySequence::cache_bucket_lenandselect_running_bucketruns exactly one bucket per step. That is correct for everything that reaches it today because the cohort rule keeps cache lengths equal — which iscache_bucket_len's own documented invariant, and per-sequence advance is precisely what deletes it. The first step with differing accept counts shatters the batch into one bucket per length and idles all but one.Granting the mode now would ship (1) as a silent wrong answer and (2) as a regression larger than the mode is worth, both while MTP went on reporting a healthy
accept_rate. It is stated asbatch_admission_admits_ragged_cohorts()so the next change flips exactly one thing — and it is one change, because both sites are the same assumption.I am not claiming this is the last blocker. Five agents have now had that claim proved wrong; the useful output of each wave has been naming precisely where the refusal moved.
Context — the honest magnitude
The prize for this whole chain is +31% at B=128 (
tok_per_step1.0558 measured → ~1.48 predicted, plan arithmetic). Per-user at B=128 is 1.09 tok/s against a 68 ceiling — a 62× gap. This chain fixes a real defect and is worth finishing, but it is a 1.3× on a 62× problem; the bulk lives elsewhere (host overhead, and the quadratic prefill in task #22). Nothing here should be read as closing the per-user gap.🔴 The GPU ask
Nothing changes a default, so this is a no-regression check on the box already serving V4. I did not run it (D15 — I never call
runcrate).The one number: decode tok/s in B as a fraction of A, at B=1 and B=128; the claim is
B/A == 1.00at both, because the refusal means B still takes the cohort path. Andgrep "cannot honour it" /tmp/b.txtmust now citecache_bucket_len/CacheInstruction::Inand must contain none ofXsRolling, the ragged mask, the fused step'scache_len, orforward_step— if it names any of them, this wave did not land.Noticed, not shipped
front_align_draft_groupsizes the batched buffer at exactly the group's width, so the chain's firstappendgrows it by oneCACHE_GROW_SIZEblock. One realloc per ragged group per step, bounded and correct; carrying each row's own slack through the concatenation would avoid it. B=1 and uniform groups pay neither (they never reach this path).front_align_batch'smodify_draft_cache: boolparameter is never passedtrueanywhere in the tree — it operates onSequence::normal_draft_cache, which is the non-MTP speculative draft cache. Wired-but-dead; worth a BACKLOG line.layers_masker::RaggedKvLensis constructed in exactly one place, a test. Its doc reads as if the ragged-mask wiring is done for every model; it is not — V4 builds its ownragged_union_maskfromcfg.row_q0. Same observation feat(v4): per-rowq0indsv4_attention— the ragged-mask blocker #92 named #100 made from the other side.