Repository navigation
speculative: --spec-draft-window (MTP drafting at every depth) - #320
professorpalmer wants to merge 3 commits into
Conversation
bri-prism
left a comment
There was a problem hiding this comment.
I found three issues that need fixing before this is ready: the standalone server does not compile, window eviction crosses sequence boundaries, and sleep/wake loses the normal draft limit. Details and focused reproductions are attached inline.
The server translation unit fails its generated CPU syntax check; the base source passes the same check. Runtime counterexamples use the extracted changed code with actual KV-cell metadata or the actual draft-limit method, under AddressSanitizer and UndefinedBehaviorSanitizer. These are focused reproductions, not an end-to-end MTP serving run, because the current head cannot compile.
| slot.spec_depth_max = params_base.speculative.draft.n_depth_max; | ||
| slot.spec_n_max = spec_n_max; | ||
| slot.spec_n_max_tail = params_base.speculative.draft.n_max_tail; | ||
| slot.spec_tail_depth = params_base.n_kv_vram_cells; |
There was a problem hiding this comment.
[P1] Remove the undeclared tiered-KV dependency from the standalone PR
common_params has no n_kv_vram_cells field on this branch. The generated server compiler command fails here with no member named 'n_kv_vram_cells' in 'common_params', including when neither new flag is used. The base source passes the same syntax check.
The field belongs to the separate tiered-KV change, so the window currently cannot work independently as described. Please make this branch compile without that prerequisite, or explicitly stack/include the dependency. The tail threshold should remain zero until a real tiered-KV API is available.
| const llama_pos hi = pos_min - params_base.speculative.draft.n_window; | ||
| if (hi > 0) { | ||
| for (auto & slot : slots) { | ||
| llama_memory_seq_rm(llama_get_memory(ctx_dft), slot.id, 0, hi); |
There was a problem hiding this comment.
[P1] Trim only batched sequences, using a separate cutoff for each
The cutoff is computed from the minimum position of the whole batch, then applied to every slot, including absent slots. With window 100 and only slot 0 batched at position 5000, cutoff 4900 deletes all 500 cached rows of slot 1 at positions 0-499. If both slots are batched at positions 5000 and 500, cutoff 400 instead leaves 1000 old cells in the deep slot where only 100 should remain.
Both cases reproduced using the exact eviction block and real KV-cell metadata/removal code. This can discard another slot's draft prefix or defeat the smaller draft context's bounded-cache assumption; continued growth can exhaust that context. Please compute the cutoff per sequence ID present in the batch and only trim that sequence.
| // the drafter and the output limits are built for the larger of the two draft sizes; each slot caps a | ||
| // draft to --spec-draft-n-max below the tiered-KV line (see server_slot::get_n_draft_max) | ||
| spec_n_max = params_base.speculative.draft.n_max; | ||
| params_base.speculative.draft.n_max = std::max(params_base.speculative.draft.n_max, params_base.speculative.draft.n_max_tail); |
There was a problem hiding this comment.
[P2] Preserve the requested normal draft limit across sleep/wake
The initial load saves the normal limit, then widens params_base.speculative.draft.n_max for allocation. On wake, handle_sleeping_state(false) calls load_model(params_base), so line 990 recaptures the already-widened value as the normal limit.
With normal limit 2 and tail limit 4, the extracted initialization code plus the actual get_n_draft_max() method returns 2 before sleep and 4 after reload at position 100, below a 32768 tail threshold. Please keep the requested limits separate from allocation capacity and avoid recapturing the normal limit from mutated parameters on resume.
…ff; drop --spec-draft-n-max-tail Review of PrismML-Eng#320: - The window cutoff was the minimum position of the whole batch and was applied to every slot, so a deep slot could remove the rows of a shallow slot that was not in the batch, and a shallow slot in the batch left extra rows in a deep one. Now each sequence in the batch gets its own cutoff, and sequences that are not in the batch keep their rows. - --spec-draft-n-max-tail needed n_kv_vram_cells from the tiered-KV change (PrismML-Eng#319), so this branch did not compile on its own. The tail flag, its per-slot fields and the n_max widening at load are removed; the tail returns stacked on the tiered-KV change, with the requested draft limits kept apart from the allocation size (the widening was also recaptured as the normal limit after sleep/wake). - --spec-draft-window help text: one sentence, no forced line break.
|
@bri-prism, thank you. All three findings are correct. Fixes are in 93fcb95: [P1] Standalone build. Correct: [P1] Eviction across sequences. Correct. The cutoff now comes from each sequence's own first position in the batch, and only sequences that are in the batch are trimmed: std::map<llama_seq_id, llama_pos> seq_pos_min; // per sequence in batch_view (all seq_id entries of each token)
...
for (const auto & [sid, pos_min] : seq_pos_min) {
const llama_pos hi = pos_min - params_base.speculative.draft.n_window;
if (hi > 0) {
llama_memory_seq_rm(llama_get_memory(ctx_dft), sid, 0, hi);
}
}Check with the extracted block (plain arrays in place of the batch and the memory call), your two cases plus two more:
[P2] Sleep/wake. Correct. The cause was the widening of Still to come here: a CUDA serving run on the RTX 4070 with two slots and MTP. One slot will be deep and one shallow, with the window set below the deep slot's position. I will report the acceptance of the shallow slot, and greedy output against one-slot runs. The GPU is busy for about two hours, so that comes later today. |
|
CUDA serving check of 93fcb95, as promised. RTX 4070, Bonsai 2 27B PTQ1_0 with the MTP head, this branch built for sm_89,
Slot 1's draft rows survive slot 0's deep batches: B2 has the same text, the same draft acceptance and the same prompt reuse as without the other slot. With the old cutoff (the minimum position of the whole batch, applied to every slot), A's batches would have removed all of slot 1's rows. |
|
This needs a small rebase: prism added a LAB_SPEC_DRAFT_N_MAX knob to the same MTP stop condition, so keep both checks with an OR. With that it builds and behaves well on an H200. With a 9B PQ2_0 plus MTP head at about 29k tokens of context, a 4096 window keeps output token-identical to plain decode, acceptance is unchanged, and the draft context drops from 40960 to 8448 cells. Speed is level there (about 362 t/s with or without the window), which I expect, since the gain should show up deeper and on smaller cards. The dp.n_max check is worth having regardless, because the MTP drafter is the only one that ignores the server's per-draft cap today. Two requests: add a comment or assert tying the depth_max-based sizing to the server not feeding the draft context past depth_max, and add a small test that runs with a short window (for example 64) so the trim path is covered. |
…fting at every depth) --spec-draft-window N: the draft (MTP) context keeps only the last N rows. The server drops older rows before feeding each batch and the draft context is sized for the window, so its cells are reused, its cache stays small and on the device, and a draft pass costs the same at any depth. The MTP head predicts the next few tokens from recent context: a 16k window accepts as many drafts as the full history at 131k. Also sizes the draft context for --spec-draft-depth-max when no window is set. --spec-draft-n-max-tail N: draft size once the sequence reaches --kv-vram-cells. Past the tiered-KV line a step is bound by reading the host tail over PCIe, and a wider verify reads it once for all columns. The drafter and the output limits are built for max(n_max, n_max_tail); each slot caps a draft by depth, and the MTP draft loop now honours that per-draft cap. Bonsai 2 27B on an RTX 4070 with the MMA decode route: drafting pays at every depth, so the --spec-draft-depth-max cutoff is no longer needed (32k 54.5 -> 103.6 tok/s, 64k 48.0 -> 90.1, same acceptance; window alone +17-18% at 131k; tail draft 4 vs 2: +26% code / +15% prose at 180k).
…ff; drop --spec-draft-n-max-tail Review of PrismML-Eng#320: - The window cutoff was the minimum position of the whole batch and was applied to every slot, so a deep slot could remove the rows of a shallow slot that was not in the batch, and a shallow slot in the batch left extra rows in a deep one. Now each sequence in the batch gets its own cutoff, and sequences that are not in the batch keep their rows. - --spec-draft-n-max-tail needed n_kv_vram_cells from the tiered-KV change (PrismML-Eng#319), so this branch did not compile on its own. The tail flag, its per-slot fields and the n_max widening at load are removed; the tail returns stacked on the tiered-KV change, with the requested draft limits kept apart from the allocation size (the widening was also recaptured as the normal limit after sleep/wake). - --spec-draft-window help text: one sentence, no forced line break.
…ze the MTP draft context for --spec-draft-depth-max only with one sequence, and assert in the server that a fed view ends below depth-max + n_batch; server test for a 64-row draft window
93fcb95 to
6f5709a
Compare
|
Thank you for the H200 run. Rebased on prism
I ran the same check with MTP on an RTX 4070 (Bonsai 2 27B with its MTP head,
0 CUDA errors, and the new assert did not fire. A CPU-only build of this branch alone also compiles and links |
One of the five pieces of #285, split per bri-prism's request on #317. Two commits on
prism(eaecb50c7): the change, and the fixes from the review. Applies without conflicts.What it does
--spec-draft-window N: the draft (MTP) context keeps only the last N rows of each sequence. Before each batch, the server drops the rows of each sequence in the batch that are older than the window, measured from that sequence's first position in the batch; sequences that are not in the batch keep their rows. The draft context is sized for the window (common_speculative_init), so its cells are reused and a draft pass costs the same at any depth. This retires the--spec-draft-depth-max 24576cutoff from #221 (the flag stays, default 0). The MTP draft loop now honours the per-draft cap that the server sets for each slot.The tail draft size (
--spec-draft-n-max-tail) is no longer in this PR: it needs the tiered-KV line from #319, so it will come as a follow-up stacked on #319. This branch now builds on its own (CPU-only build ofllama-serverchecked); a CUDA serving check with two slots follows in the thread.Receipts (RTX 4070, Bonsai 2 27B PTQ1_0 with the MTP head, q8_0 K/V)
ggml_cuda_fattn_mma_kv_native_supportedguard); the window alone removes the depth cutoff, the speed at depth depends on the attention kernel the decode step uses.Serving recipe and receipts: https://github.com/professorpalmer/bonsai-ada-surgery/blob/main/docs/Q8_FULL_CONTEXT.md