Repository navigation
feat(ArcSched/ArcKV): ragged dense decode — removes the decode-side length limiter [MEASURED, gated by prompt admission] - #122
Conversation
`Sdpa::run_attention`'s CUDA branch calls
flash_attn(&q, &k, &v, flash_params, sdpa_params)
which takes NO mask argument. Whatever the caller passed in `mask` is
silently discarded there. The flash kernels' only masking knobs — `causal`
and `window_size_left/right` — are batch-scalar, so a mask that varies
across the batch is inexpressible on that path by construction.
This matters because the tree already contains such a mask, fully built and
fully tested, that would have been a silent no-op:
`CausalMasker::make_left_padded_causal_mask` produces the `[B, 1, 1, k]`
mask a left-aligned ragged decode batch needs to kill each row's zero-filled
dead prefix. A zero K row is not a masked row — it scores logit 0 and takes
real softmax weight — so handing that batch to flash would serve every short
sequence from padding, with nothing downstream catching it.
`MtpSpeculativePipeline::target_masks_ragged_batches` names the blocker as
"no model in this tree threads a ragged-batch mask into its forward yet".
That is a hardcoded `Err(...)` which probes nothing, and it points at the
wrong work: threading the mask through every model would not have helped
while the dispatcher discarded it.
The predicate is deliberately narrow — `seq_len == 1` only — so it cannot
move anything that works today:
* a uniform decode batch has no mask at all;
* prefill is untouched, including the 4-D padding masks the vision encoders
build with `expand_mask`. Those are ALSO dropped on the flash path today;
that is a separate pre-existing defect, recorded rather than fixed here,
because widening this predicate to cover it would move every vision
encoder off flash in the same change.
Seen red first: with the predicate forced false, the ragged case fails on
`assertion failed: mask_must_be_applied_as_bias(1, Some(&[8, 1, 1, 500]))`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…op bucketing The dense scheduler ran exactly one cache-length bucket per step, so a length-diverse cohort decoded one sequence at a time. This makes a ragged cohort share one forward. Three of the four pieces already existed and were unreachable: `front_align_batch` (left-aligns a ragged cohort so every row's live K/V ENDS at the same column, making `SingleCache::append`'s one shared write offset correct for all of them), `make_left_padded_causal_mask` + `RaggedKvLens`, and `KvCache::supports_per_sequence_len`. Only MTP called any of it, and it discarded the result. What was missing, and why the code named the wrong blocker. `MtpSpeculativePipeline::target_masks_ragged_batches` is a hardcoded `Err` saying "no model in this tree threads a ragged-batch mask into its forward yet". Threading it through all forty-odd models would NOT have worked: `Sdpa::run_attention`'s flash branch calls `flash_attn(&q,&k,&v,..)`, which takes no mask argument, so the mask was discarded at the dispatcher. That is fixed separately in this branch's first commit. This commit adds the rest: * `RAGGED_LEAD_PAD`, a thread-local carrying each row's dead prefix, set by `clone_in_cache`. `CausalMasker::make_causal_mask_matrix` reads it, so the ragged mask reaches every model without a new argument anywhere. It holds the PREFIX, not the live lengths, because `clone_in_cache` runs only when batch membership changes while the cache grows every step — a stored `live` would be stale on the next token, whereas `lead_pad` stays true and `live[i] = past_kv_len - lead_pad[i]` is recovered from the cache itself. * `clone_in_cache` front-aligns a mismatched batch instead of refusing it, when every slot can carry its own length. `ensure_uniform_batch_cache_lens` is kept as a hard postcondition. * `clone_out_cache` hands each row back its OWN length, dropping the dead prefix. `SingleCache` is start-anchored, so the prefix has to be removed rather than merely excluded by a length. Identity when `lead == 0`. * The scheduler pins the length component of the bucket key to 0 for DECODE when the cache can carry per-sequence lengths. Prompts keep the exact-length key: prefill is right-padded and its `cu_seqlens` come from those padded widths, a different mechanism.⚠️ A guard's contract narrowed, deliberately. `clone_in_cache` no longer refuses every length-mismatched batch — only those it cannot align. The wave51-CB incident guard (refuse in release, return rather than panic, never reach `slice_set`) is preserved and now covered by a `Rotating`-slot fixture. Two flakiness bugs of my own, both caught by the suite and both fixed rather than worked around: the capability flag is process-global, so tests flipping it corrupted a concurrently running pre-existing test (fixed with a thread-local test override mirroring `xs_rolling::test_override`), and one of my new tests asserted that same global (replaced with the pure predicate). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the guard narrowing found a real hazard in it, and this closes it. `front_align_batch` pads IN PLACE, sequence by sequence and slot by slot. So "align, then assert uniformity" could return `Err` having already rewritten the rows ahead of whichever one failed — an unmaterialised buffer, or a row whose padded capacity would exceed its `max_seq_len`. The blanket refusal it replaced ran BEFORE anything was touched and mutated nothing on the way out, and that property is part of what makes a refusal safe: the engine can fail those requests without the surviving sequences carrying a half-aligned cache. `front_align_would_succeed` restores it. Alignment is attempted only when it is known to complete; otherwise the batch falls through to the original refusal, unmutated, exactly as before. Pinned by `a_refusal_does_not_leave_a_half_aligned_batch`, whose fixture is the precise shape that bails midway — sequence 0 paddable, sequence 1 not. Exit analysis, since a postcondition on the happy path is not a postcondition: between the alignment decision and `slice_set` there is now exactly one fallible call, `ensure_uniform_batch_cache_lens`, and its `?` leaves the function without reaching `slice_set`. `front_align_batch`'s own `?` is retained as a hard error but is no longer reachable with a batch that would fail it. Also: mutation testing showed `batch_can_be_ragged` could be replaced with `true` and the entire suite stayed GREEN — `front_align_would_succeed` independently refuses the same batches. The alignment decision is double-covered; the CAPABILITY, which is what the scheduler reads to decide whether to stop bucketing at all, was covered by nothing. Now pinned by `the_capability_predicate_is_what_the_scheduler_reads`, and the deliberate overlap is recorded at the call site. Verified in release as well as debug: the refusals return rather than panic, and 438/438 pass under `--release`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y point too Caught on hardware by the completeness guard, not by a test: the fix arm returned 433 of 512 requested tokens at B=8 spread, 1518/2048 at B=32, 7956/8192 at B=128 — with ZERO reported errors, and with every UNIFORM cell exact. Short, early-terminating generations, which is what wrong tokens look like from the outside. Cause. `CausalMasker` has two entry points, and the ragged channel was only checked in one. Qwen2 reaches the other: its `Config` reads `sliding_window` and, unlike Qwen3/Qwen3-MoE, ignores `use_sliding_window`, so Qwen2.5-0.5B loads with `Some(32768)` even though its HF config says the window is off. `make_sliding_window_causal_mask_matrix` then took the SWA branch, returned `None` at `tgt_len == 1`, and every ragged decode batch attended its zero-filled dead prefix — logit 0, real softmax weight, silently wrong. Fixes: * the channel check is factored into `ragged_mask_from_channel` and called from BOTH entry points, before every early return, so which mask function a model happens to call can no longer be the difference between a masked dead prefix and an unmasked one; * `make_left_padded_causal_mask` takes the window and applies it ON TOP of the dead prefix. The window is in absolute positions and the buffer is shifted by `lead`, so its low edge shifts with it: key `j` sits at absolute `j - lead`, giving `j > last - w`; * `set_none_cache` clears the channel. A prompt step takes `CacheInstruction::Reset`, i.e. that function, so a torn-down cohort's geometry would otherwise have been handed to the next batch. Pinned by `the_sliding_window_entry_point_also_honours_the_ragged_channel`, which asserts the dead prefix is masked on the SWA path AND that the window still bites on a fully-live row AND that no row is entirely masked. The throughput numbers from that run are discarded, not reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ds it `clone_out_cache` runs on every decode token, but the per-sequence caches it writes are only re-read when batch membership changes (`pre_op` is `In` only when `last_completion_ids != current_completion_ids`); in between, the forward runs off the batched cache the pipeline still holds. Dropping each row's dead prefix there was therefore `O(B x layers x 2)` device copies PER TOKEN for data nobody looks at — about 2,200 copies per token at B=47. Now `clone_out_cache` records the prefix per sequence id (no tensor work) and `clone_in_cache` pays for it once, before any length is compared or any alignment decided. Uniform batches record nothing and are unchanged. This is also the experiment: the per-step strip is the leading suspect for the 15.7% regression measured at B=128 spread, and removing it confirms or eliminates that by construction. Profiling it first would have cost the same and still left this edit to write.⚠️ KNOWN GAP, must close before merge: between `clone_out_cache` and the next `clone_in_cache`, a sequence's own cache still carries its dead prefix and reports a length that includes it. Anything that reads a sequence's cache outside that window sees padded content — the prefix cacher on completion is the case that matters. Measured with `--prefix-cache-n 0`, so the numbers are unaffected, but this is not yet safe to merge with prefix caching enabled. Also carried: a null (baseline vs baseline, three samples) puts the run-to-run floor at about +/-2% on every cell, so deltas under ~5% in this harness are not readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e inversion B=32 uniform read -5.1% then -9.7% in the ladder and looked like a regression. It is not one. `a_uniform_batch_leaves_every_ragged_mechanism_untouched` asserts what was previously only reasoned: on a uniform cohort `clone_in_cache` publishes NO dead prefix, leaves ZERO deferred-strip entries, and the masker still returns `None` at `tgt_len == 1`. Any non-zero there would have located the regression; all three are zero, which eliminates the ragged path. Five samples per arm on that cell alone then inverted the reading: baseline 1157.50 / 887.60 / 885.79 / 874.68 / 876.96 mean 936.51 sd 123.66 fix 1186.59 / 1200.91 / 1212.11 / 1199.62 / 1212.26 mean 1202.30 sd 10.62 The fix is +28.4% on the mean and 12x more stable. The variance is the BASELINE's. It runs fast once, then collapses to ~880 and stays there, because the repeat harness drains with a 40-token probe whose length differs from the 32 identical ones — so the "uniform" cell is not uniform, a ragged cohort forms, and the baseline serialises exactly what this change merges. Consequence recorded in the wave note: the ladder has ORDER EFFECTS. The same fix reads 1075-1130 inside the sweep and 1186-1212 in isolation, because the two arms arrive at B=32 in different states. The +/-2% cross-server null measures server-start variance and does not cover this, so every ladder delta under ~10% is unresolved until re-measured in isolation. Still merge-blocking and unchanged: the deferred strip leaves a sequence carrying its dead prefix between `clone_out` and the next `clone_in`, so the prefix cacher on completion would read padding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…efix cache The last merge-blocker on this branch. Deferring the ragged strip left a window: between `clone_out_cache` and the next `clone_in_cache`, a sequence still carries its zero-filled dead prefix and reports a length that includes it. For a sequence that has just FINISHED there is no next `clone_in_cache` — so `PrefixCacheManagerV2::add_sequence` stored the padding and would hand it to every future request matching that prefix. A wrong answer that outlives the request that produced it, and one that `--prefix-cache-n 0` hides completely, which is exactly why the measured runs did not show it. Closed at the READER, not the writer: `add_sequence` strips the pending prefix before it reads the cache. That is one sequence at a read instead of `O(B x layers)` every token at the write, so the deferral (worth 675 -> 764 tok/s at B=128 spread) is kept and its only unsafe consumer is paid off precisely. On failure it refuses to store rather than storing padding. `clone_out_cache` cannot do this itself: it runs BEFORE sampling (`pipeline/mod.rs:976`), so no sequence knows yet whether it has finished. Seen red by mutation: with the strip removed the new test reports `left: 64, right: 60` — four zero-filled K/V rows cached as content. 441/441 green in debug and release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Master moved 10+ commits since 120f348, including #100 (v4 ragged mask wire) and #102 (per-row cache_len in the fused MTP step) — adjacent territory. Merged now rather than letting the divergence be discovered later. Clean auto-merge; the only overlapping file is `kv_cache/mod.rs`. 466/466 green on the merged tree (441 mine + master's new tests). 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 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
…ed five cells that never ran this file PR #123 reported that the 47 ceiling 'did not reproduce' and that the floor changed nothing. **Both conclusions are void.** Every cell was served with PagedAttention live — the server's own log says `Allocating 127285 MB for PagedAttention KV cache` — so the engine used `PagedAttentionScheduler` and `default_scheduler.rs`, which holds the length bucketing, the dense chain's decode pin, my cap and my floor, **never executed**. PR #122 specifies `--paged-attn off`; I did not pass it. My completeness guard checked errors, server-alive, tokens > 0 and admission telemetry — and none of those can tell 'the mechanism did nothing' from 'the mechanism was not on the code path'. That is the exact failure I have been asserting against in other people's work all session, in my own harness. The fix is a POSITIVE signal, not a tighter absence-check: the floor logs once when it first forces a prompt bucket, and the cap logs once when it first holds a sequence back. A cell whose arm claims a mechanism and cannot show its line is now failable, and the harness fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Your Two things you should have from this chain. 1. It is starvation, not a throttle, and my earlier framing (relayed to you) Measured on your configuration — Qwen2.5-0.5B,
Baseline repeated across passes: 867.98 / 857.84, 1.2% spread. Your ceiling 2. The trap that cost me a published result, so it does not cost you one. Both guards are now in the harness and are worth copying: a path gate that 3. A free observable you were told you didn't have. Also inherited gratefully and used unchanged: no drain probe (fresh server per |
Removes the decode-side length limiter on the dense scheduler. Merged with current master (clean; 466/466 green on the merged tree).
This PR is not claiming the flagship 2.84×, and the headline number should not come from here. What this removes is one of two limiters in series. The other — prompt admission — is a separate fix by a separate chain and is not yet combined with this one. Every number below was measured with that second limiter still in front of this one, so they are a floor, not the result. The number worth quoting comes from the combined run.
What it does
The dense scheduler ran exactly one cache-length bucket per step, so a length-diverse cohort decoded one sequence at a time. Measured effect on the scheduler itself, from the engine's own log at B=128 spread:
It does not reach 128 because prompt admission gates it — see above.
Three pieces already existed and were unreachable
front_align_batch,make_left_padded_causal_mask+RaggedKvLens, andsupports_per_sequence_len. Only MTP called any of it, and it discarded the result.🔑 The code named the wrong blocker.
MtpSpeculativePipeline::target_masks_ragged_batchesis a hardcodedErrreading "no model in this tree threads a ragged-batch mask into its forward yet". Threading it through all forty-odd models would not have worked:Sdpa::run_attention's flash branch callsflash_attn(&q,&k,&v,..), which takes no mask argument, so the mask was discarded at the dispatcher. That is fixed first (mask_must_be_applied_as_bias, scoped toseq_len == 1so uniform decode and every prefill are untouched).Then: a thread-local carrying each row's dead prefix, set by
clone_in_cacheand read byCausalMasker— which is what gets the mask to every model without a new argument anywhere;clone_in_cachefront-aligns instead of refusing;clone_out_cachehands each row back its own length; the scheduler pins the length key to 0 for decode only (prompts keep the exact-length key — prefill is right-padded and itscu_seqlenscome from those padded widths, a different mechanism).The channel carries the prefix, not the live lengths:
clone_in_cacheruns only when membership changes while the cache grows every step, so a storedliveis stale on the next token, whereaslead_padstays true andlive = past_kv_len - lead_padis recovered from the cache.Measured
H200, Qwen2.5-0.5B,
--paged-attn off,--prefix-cache-n 0, exclusive card.BASE=120f34875/FIXproved withgit merge-base --is-ancestorand a printed commit count — not a relative ref (see Method notes).Unresolved, named:
+6.1%at B=32 spread and+5.1%at B=128 uniform. Both are ladder measurements below the readability bar and are not claimed. See Method notes.The B=32 uniform story, because it inverted
The ladder read this cell as −5.1%, then −9.7% — a regression. It is not one.
First, inertness was proven, not assumed (
a_uniform_batch_leaves_every_ragged_mechanism_untouched): on a uniform cohortclone_in_cachepublishes no dead prefix, leaves zero deferred-strip entries, and the masker still returnsNoneattgt_len == 1. Any non-zero there would have located the regression. This is the test a reviewer should read to trust that the change is genuinely inert where it should be. The other candidate — the scheduler pin colliding with prompt buckets — is excluded by arithmetic: a fresh prompt'scache_bucket_lenislen()-1≈ 274 and cannot equal the pinned 0.Then five samples per arm, that cell alone, one server per arm:
The baseline carries the variance: it runs fast once, then collapses to ~880 and stays. Cause: the harness drains with a 40-token probe before each rep, while the cell sends 32 × ~275-token prompts — so the "uniform" control was ragged, and was measuring exactly the thing under test. The baseline serialises it; this change merges it.
Correctness
B × 64tokens.The merge-blocker that was closed, and how
Deferring the strip left a window: between
clone_out_cacheand the nextclone_in_cachea sequence still carries its zero-filled dead prefix and reports a length that includes it. For a sequence that has just finished there is no nextclone_in_cache— soPrefixCacheManagerV2::add_sequencewould store the padding and hand it to every future request matching that prefix. A wrong answer that outlives the request that produced it, and one--prefix-cache-n 0hides completely.Closed at the reader, not the writer.
add_sequencestrips the pending prefix beforeseq.normal_cache().to_vec()— one sequence at a read, instead ofO(B × layers)every token at the write. That keeps the deferral (worth 675 → 764) and pays off its only unsafe consumer precisely. On failure it refuses to store rather than storing padding.🔑 Why the writer was not available:
clone_out_cacheruns before sampling (pipeline/mod.rs:976), so at that point no sequence knows whether it has finished — "strip only the finishing ones at the write" is not expressible there.Seen red by mutation — with the strip removed:
i.e. four zero-filled K/V rows cached as content and keyed as if real.
A guard's contract narrowed, deliberately
clone_in_cacheno longer refuses every length-mismatched batch — only those it cannot align. wave51-CB's guarantee (refuse in release, return rather than panic, never reachslice_set) is preserved and now covered by aRotating-slot fixture, withensure_uniform_batch_cache_lenskept as a hard postcondition.Review of that narrowing found a real hazard:
front_align_batchpads in place, so it could returnErrhaving already rewritten the rows ahead of the failure, where the blanket refusal it replaced mutated nothing.front_align_would_succeedrestores the property — alignment is attempted only when known to complete. Pinned bya_refusal_does_not_leave_a_half_aligned_batch.Mutation results, both run:
batch_can_be_raggedwithtrue⇒ suite stayed green. The alignment decision is double-covered byfront_align_would_succeed; the capability — what the scheduler reads — was covered by nothing. Now pinned bythe_capability_predicate_is_what_the_scheduler_reads.A defect this found in itself, worth a reviewer's attention
The first measured run returned 433/512 tokens at B=8 spread with ZERO reported errors and every uniform cell exact.
CausalMaskerhas two entry points and the channel was wired into one; Qwen2 reaches the other, so every ragged decode batch attended its zero-filled dead prefix. Only the completeness guard caught it — the harness exited 0.🔑 The family: a new channel wired into one of two dispatch paths, where which one you hit depends on model config. When adding a channel, enumerate every entry point and every early return, and make sure a fixture reaches each. The suite was green throughout; it could not reach the second path because no fixture had that config.
Method notes a reviewer should not have to reconstruct
FIX~3— correct at three commits, silently wrong at four, where it resolved to this branch's own first commit and would have put half the change under test inside the "baseline". Both SHAs are now literals and the build script proves the pair.CANNOT-ANSWER: fewer than 4 distinct B=1 completions— a 0.5B model summarises eight numbered copies of the same filler identically, so "8/8 identical" would have been vacuous.Unrelated defect found on the way, not fixed here
models/qwen2.rs'sConfigdeserialisessliding_window: Option<usize>and never readsuse_sliding_window, whileqwen3.rs:33,qwen3_moe.rs:33andembedding_models/qwen3_embedding.rs:28all gate on it. ⇒ every Qwen2-architecture model runs with sliding-window attention ON when its config says OFF. Verified on Qwen2.5-0.5B-Instruct (sliding_window = 32768,use_sliding_window = false, engine takes the SWA mask path). Mostly inert while the window ≥ served context, which is why it has not surfaced — but it will bite on any Qwen2 model whose window is smaller than its context. Not fixed here because it is not this change's territory.Full record:
memory/mission/wave64-CP-dense-ragged-decode.md.🤖 Generated with Claude Code