Repository navigation
server: a KV handoff records its cells as an in-place checkpoint, so the decoder stops re-processing the prompt (engine#941) - #189
Conversation
…the decoder stops re-processing the prompt (engine#941) ?action=restore and ?action=adopt put a slot's cells in place, but the prompt-cache search in update_slots only consults slot.prompt.checkpoints. With no checkpoint it resets n_past to 0 and the whole prompt is re-processed - the 27B hybrid's handoff buys nothing end to end (engine#941: 287 / 847 / 3 286 tokens re-evaluated for 287 / 847 / 3 286-token prompts). "leave_one_token": true in the restore/adopt body says the client leaves the prompt's last token for this context to evaluate, which makes a data-less checkpoint legal: it carries pos_min/pos_max/n_tokens and no state buffer, and common_prompt_checkpoint::load_tgt returns at once for it. The search then lands n_past on the handoff, the request's last token is the one evaluated, and the token list stays consistent with the memory (the alternative - claiming the whole prompt - makes the server trim the memory back by one position, which llama_memory_recurrent::seq_rm refuses, aborting the process). Off by default on the request side and skipped for a sliding-window model (llama_model_n_swa > 0), whose cache is a window rather than the prefix and has no data to be restored from here. Closes ggml-org#941
|
#189 ready for review at a912281 (engine 1bit-MONSTER/engine#967 is the other half) |
… fork#189 adds (review on #967) pi-prs: the docs described behaviour the pin does not have. At d8bd0ff60 nothing reads leave_one_token - handle_slots_restore takes only filename, handle_slots_adopt only tokens and share_id - so the field is silently ignored there and the one-token saving comes from the ordinary prompt-cache prefix search. The in-place checkpoint (and reuse across later requests) is fork 1bit-MONSTER/llama.cpp#189, which the pin does not carry. The paragraph now says that in those words. The two route tests are also labelled for what they are: they pin the request shape, not the effect, so nobody reads them as evidence that the checkpoint path runs at the pin.
…off is not thrown away (engine#941) (#967) * serve: leave the prompt's last token to the decoder, so a handoff is not thrown away (engine#941) The hybrid route handed the provider the whole prompt and then restored/adopted that state. A hybrid model's recurrent memory keeps its prefix in one cell, so the backend cannot trim it back by the one position its cache path needs ([TAG_PROMPT_LOGITS]), and the handoff was discarded: the decoder re-processed the entire prompt (engine#941: 287 / 847 / 3 286 tokens for prompts of the same length). The provider is now handed the prompt minus its last token, and restore/adopt say "leave_one_token": the decoder evaluates that one token from state that is genuinely in place. The flag is what lets the pinned server record the handoff as an in-place (data-less) context checkpoint instead of resetting n_past to 0 - fork 1bit-MONSTER/llama.cpp#<fork-pr>. One token on the decoder replaces the prompt. Prompts shorter than 2 tokens have nothing to hand over and keep the old fallback; a sliding-window model is refused the checkpoint by the fork side and re-processes as before. Closes #941 * docs: say what the pinned backend does with leave_one_token, and what fork#189 adds (review on #967) pi-prs: the docs described behaviour the pin does not have. At d8bd0ff60 nothing reads leave_one_token - handle_slots_restore takes only filename, handle_slots_adopt only tokens and share_id - so the field is silently ignored there and the one-token saving comes from the ordinary prompt-cache prefix search. The in-place checkpoint (and reuse across later requests) is fork 1bit-MONSTER/llama.cpp#189, which the pin does not carry. The paragraph now says that in those words. The two route tests are also labelled for what they are: they pin the request shape, not the effect, so nobody reads them as evidence that the checkpoint path runs at the pin. --------- Co-authored-by: pi-zerocopy <zerocopy@1bit.local>
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
READY, unchanged from my earlier read at this head (a9122817b) — and the PR's own admission is what makes it safe to land: it is bookkeeping, opt-in, and it does not claim to change any measured outcome.
What I checked then and re-checked now: leave_one_token is read on both endpoints (request_data.value("leave_one_token", false) in handle_slots_restore, body.value(..., false) in handle_slots_adopt), i.e. exactly the key the engine's hybrid route sends, and both default to false so a plain restore/adopt is byte-for-byte the old behaviour. create_inplace_checkpoint() is guarded by n_tokens > 0, n_ctx_checkpoints > 0 and the sliding-window refusal, and it pushes a checkpoint with no data — the shape common_prompt_checkpoint::load_tgt already returns from immediately.
The one thing worth repeating for the morning pass: the engine half (ggml-org#967) is what closes engine#941, so this can land before or after it without changing a number; the pin bump that makes either side effective is a separate step.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
a9122817b, 2 files (+44). A small bookkeeping change whose correctness rests on two facts I verified in the source,
and on the prompt-cache arithmetic, which I re-derived.
load_tgt really is a no-op with no data — the claim the whole design depends on:
void common_prompt_checkpoint::load_tgt(llama_context * ctx, llama_seq_id seq_id, llama_state_seq_flags flags) const {
if (ctx == nullptr) return;
if (data_tgt.empty()) return; // <- an in-place checkpoint restores nothing, by designso a checkpoint with no data_tgt is an existing shape, not a new one, and create_inplace_checkpoint can register one
without copying anything into it.
The arithmetic that makes leave_one_token exactly one token. With cur.update_pos(n_tokens, 0, n_tokens - 1), the
search does:
pos_next = min(pos_next, max(it->pos_min + 1, it->pos_max)) = n_tokens - 1
n_past = min(slot.prompt.tokens.size_up_to_pos(pos_next), it->n_tokens) = n_tokens - 1
i.e. the decoder starts at the last handoff token, which is the caller's promise. And the guard that makes a shorter or
diverging request safe is upstream's own, not something this PR had to add: if (cur.pos_max > pos_next) return false;
— a request that wants fewer positions than the checkpoint claims is rejected, so the handoff's cells can never be
treated as a shared prefix for a prompt that does not continue them.
The three refusals are the right three: n_tokens <= 0, no target ctx, n_ctx_checkpoints <= 0; the SWA model is refused
with a reason (a sliding-window cache holds a window, not the prefix, so [0, n_tokens-1] would be a lie) — which is
notably the case upstream's own create_checkpoint still carries a TODO about; and the eviction loop matches the
existing one's shape (front-erase while at the cap).
Compile at this head: tools/server/server-context.cpp with this PR's tools/server/server-task.h (both needed —
the header carries leave_one_token) -> rc 0, 0 errors, 0 warnings.
Not run by me: an actual handoff round-trip on a live server (/slots restore + kv-share adopt, then a request), which
is what would exercise the restore end to end on a model with a KV cache. The two halves it rests on are the ones above.
Status: this is not required to fix engine#941 — measured, not assumed
When I opened this PR it was engine#941's "other half": the claim was that without a backend-side checkpoint the
prompt-cache search resets
n_pastto 0 and the decoder re-processes the whole prompt. That claim is wrong for thefix as shipped, and I measured it rather than leaving it in the record:
gpu-hold -l zerocopy)leave_one_tokenstrings in the running.so(pind8bd0ff60, 04:30:57-04:33:51)--device hrxa9122817b, 02:12-02:17)Handing the decoder a prompt that is one token short is enough: the ordinary prompt-cache prefix search finds the
handoff,
n_pastlands on it, and one token is evaluated. Both arms:need to evaluate at least 1 tokenlines 0,gfxhub faults 0, restore 25-57 ms. So engine#941 is closed by the engine change alone (engine#967), and I have no
measurement in which this PR changes an outcome.
What it does add, for whoever decides whether it is worth landing
"leave_one_token": trueon?action=restore/?action=adoptrecords the handoff as an in-place contextcheckpoint: bookkeeping only, no state buffers, so its restore is a no-op by construction
(
common_prompt_checkpoint::load_tgtreturns at once whendata_tgtis empty). That makes the handoff visible to thecheckpoint path, which the prefix search is not — i.e. reuse of handed-over cells in the cases where the prefix
search does not apply (slot tokens trimmed by
keep_first, a cache invalidated, a prompt that diverges earlier thanthe handoff). The engine's hybrid route is one serialized slot that re-hands-over every request, so it does not need
that today.
Opt-in and default-off on the request side: a plain
?action=restore/adoptbehaves exactly as before. Skipped for asliding-window model (
llama_model_n_swa > 0), whose cache is a window rather than a prefix.The trap it also documents
Making a checkpoint claim the whole prompt instead (the first version of this patch) aborts the process: the server
then trims the memory back by one position to leave a token to evaluate (
slot.mem.seq_rm(id, pos_next, -1)), andllama_memory_recurrent::seq_rmrefuses a partial erase —failed to remove sequence 0 with p0=271, p1=-1. That iswhy the flag exists at all and why the engine leaves a token.
Verification on this branch
prompt_n1 / 1 / 1, Qwen3-4B shared routeprompt_n1,answers identical to fresh and to
--device hrx, 0 faults.tests/hybrid_e2e.sh(real llama-server, Qwen3-0.6B) identical reply to--device cpu,prompt_n1 vs 22.Recommending it be judged as an optional follow-up, not as engine#941's dependency. If the owner would rather not carry
a backend field nothing currently needs, closing this loses nothing that is measured.