server : allow cache reuse for text-only prompts with mmproj loaded - #60
server : allow cache reuse for text-only prompts with mmproj loaded#60Piggidragon wants to merge 4 commits into
Conversation
|
Reviewed head: Review outcome: request changes. The implementation is narrow, follows the accepted capability-versus-content direction, builds successfully, and passed focused runtime validation. I found one blocking repository-policy issue and two review-readiness issues. I did not find an actionable correctness, security, portability, lifetime, or architectural defect in the implementation. Blocking
The raw metadata of commit Repository policy requires AI contributions to use Will slow review
The change adds the content-based gate at server-context.cpp lines 3390-3404, then moves KV positions and rewrites cached prompt tokens at server-context.cpp lines 3420-3434. Before this PR, an mmproj-loaded slot could not enter this path. The PR changes no tests, and no server test at the reviewed head references The existing
The closely analogous upstream fix, ggml-org/llama.cpp PR #25076, also changed capability-based gating to content-based gating and included regression tests.
server-context.cpp lines 3392-3393 splits one sentence after a semicolon. This directly conflicts with the no-mid-sentence-wrapping rule in AGENTS.md line 89. Make each line a complete sentence or remove commentary that the predicate already makes clear. NitsNone. Changed-line review and design assessmentThe content-tracking helper is at server-common.h lines 223-225. It reads the existing The relaxed mutation assertion is at server-common.cpp lines 631-634. The call site reaches The global cache-reuse disable is removed at server-context.cpp lines 1209-1215. The per-request check still requires The media map is cleared, trimmed, cloned, serialized, and restored with the token list in the existing Upstream precedent and duplicate searchThe direct documented need is issue #21133, which explicitly identifies Relevant upstream PRs:
Separate plain-term At pristine upstream commit Exact-head build and CII built Configuration: Release, cmake --build /tmp/llama-pr60-review.liQWzP/build --target llama-server -j 8CI interpretation:
Focused runtime reproductionI used the exact-head CPU build with the official Text-only reuse with mmproj loaded: LLAMA_MEDIA_MARKER='<__media__>' /tmp/llama-pr60-review.liQWzP/build/bin/llama-server -m /tmp/llama-pr60-review.liQWzP/cache/models--ggml-org--tinygemma3-GGUF/blobs/7566ae7219c93ea2ecc692a931ee122d30c55261d0e2c3347acb8b939d2e9abd --mmproj /tmp/llama-pr60-review.liQWzP/cache/models--ggml-org--tinygemma3-GGUF/blobs/93c2ba8c34574dd8f2dfda64931fc20943de2f941bfe03e6e9eca68951b80604 --cache-reuse 4 -c 1024 -np 1 -ngl 0 --host 127.0.0.1 --port 18081 --verbosecurl -sS http://127.0.0.1:18081/completion -H 'Content-Type: application/json' -d '{"prompt":"REMOVE THIS PREFIX Alpha beta gamma delta epsilon zeta eta theta iota kappa lambda mu.","n_predict":0,"temperature":0,"id_slot":0}'
curl -sS http://127.0.0.1:18081/completion -H 'Content-Type: application/json' -d '{"prompt":" Alpha beta gamma delta epsilon zeta eta theta iota kappa lambda mu.","n_predict":0,"temperature":0,"id_slot":0}'The server logged: The second response reported I then disabled the separate RAM prompt cache so it could not replace a media-bearing slot with an older text-only state: LLAMA_MEDIA_MARKER='<__media__>' /tmp/llama-pr60-review.liQWzP/build/bin/llama-server -m /tmp/llama-pr60-review.liQWzP/cache/models--ggml-org--tinygemma3-GGUF/blobs/7566ae7219c93ea2ecc692a931ee122d30c55261d0e2c3347acb8b939d2e9abd --mmproj /tmp/llama-pr60-review.liQWzP/cache/models--ggml-org--tinygemma3-GGUF/blobs/93c2ba8c34574dd8f2dfda64931fc20943de2f941bfe03e6e9eca68951b80604 --cache-reuse 4 --cache-ram 0 -c 1024 -np 1 -ngl 0 --host 127.0.0.1 --port 18081 -lv 2curl -sS http://127.0.0.1:18081/completion -H 'Content-Type: application/json' -d '{"prompt":{"prompt_string":"What is this: <__media__>\n","multimodal_data":["iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII="]},"n_predict":0,"temperature":0,"id_slot":0}'
curl -sS http://127.0.0.1:18081/completion -H 'Content-Type: application/json' -d '{"prompt":" Alpha beta gamma delta epsilon zeta eta theta iota kappa lambda mu.","n_predict":0,"temperature":0,"id_slot":0}'Both the incoming-media request and the following text request with media cached logged: Their response timings were respectively No existing top-level comment or review duplicated these findings at the time of review. |
f3d4d77 to
732f0b4
Compare
|
Automated preliminary review by Codex; the repository owner plans a separate manual review. Verdict: FAIL Blocking
Will slow review
Nits None. Product code
Developmental progress The prohibited AI co-author trailer and production comment from the earlier revision were fixed. Coverage was added, but it is currently failing and incomplete. Reviewed head: 732f0b4 |
has_mtmd only means an mmproj is loaded, not that the current prompt carries media. Loading an mmproj disabled cache reuse for the whole server, so text-only requests lost prompt cache reuse too. Gate cache reuse on real media chunks in the cached prompt and the new prompt instead. It stays disabled once an image or audio chunk is present, and works again as soon as both prompts are text-only. Assisted-by: Claude Sonnet
732f0b4 to
c639ce2
Compare
Cache reuse is no longer disabled at startup for mmproj, so the per-slot warning now fires on every request that carries media. Warn only when the memory cannot shift, and log the expected media case at debug level. Also simplify the set_token assert: a non-empty media map already implies has_mtmd. Assisted-by: Claude Opus 5
Review follow-ups on the cache reuse block: - the shift is applied to the draft context too, so require both contexts to support it - skip reuse while an alora is invoked, the loop moved n_past past the cap - drop the context checkpoints after a shift, they no longer match the cache - set_token asserts on the media placeholder instead of the media map, and takes a token index, which is what the only caller passes Assisted-by: Claude Opus 5
One server instead of two. The shared chunk now starts on a newline, so it tokenizes the same with and without the leading sentence. Assisted-by: Claude Opus 5
|
Confirming this from a real deployment, in case a second data point helps. Running Ornith-1.5-35B-A3B on Worth recording for anyone who reads that log line and worries: ordinary prefix caching is unaffected. On a repeated 62,449-token prompt the slot still matches and reprocesses 4 tokens, TTFT 0.085 s against 26.6 s cold. What is lost is only the mid-prompt divergence case, so the practical impact is narrower than "loses prompt cache reuse" suggests — but it is real for any client that edits or trims history. Hardware: RTX 5060 Ti 16 GB, CUDA 13.3. |
3 participants
Overview
Loading an
--mmprojcurrently disables--cache-reusefor the whole server. The reuse gate keys onserver_tokens::has_mtmd, which only means "an mmproj is loaded", not "this prompt contains media". So a text-only request against a multimodal model loses prompt cache reuse even though nothing about it needs media handling.This changes the gate to look at real media chunks instead:
server_tokens::has_media_chunks()(!map_idx_to_media.empty())params_base.n_cache_reuse = 0when an mmproj is loadedcan_cache_reuseon!slot.prompt.tokens.has_media_chunks() && !input_tokens.has_media_chunks()so reuse only runs when both the cached prompt and the new prompt are text-onlyserver_tokens::set_tokenfromGGML_ASSERT(!has_mtmd)toGGML_ASSERT(!has_mtmd || map_idx_to_media.empty()), since the reuse loop calls it and it now runs with an mmproj loadedGGML_ABORT("not supported by multimodal")and thehas_mtmdassert inside the reuse blockOnce an image or audio chunk is in either prompt,
can_cache_reuseis false and behaviour is unchanged. Context shift and the global mmprojctx_shiftdisable are left as-is.Additional information
Addresses the cache-reuse part of ggml-org#21133 (capability flag
has_mtmdconflated with "media present in this conversation"). Slot save/restore and checkpoints for text-only + mmproj are out of scope here.Requirements
🤖 Generated with Claude Code