Repository navigation
batch: migrate speculative, mtmd and server to batch_ext - #29385
Conversation
|
Tested against master on text, vision, embeddings, MTP and DFlash: no regression on my side. Is n_pos in common_batch meant to be MROPE only, the lib and mtmd also count 4 positions for IMROPE? And should the common_batch call sites check the negative return of add()/add_embd() like the raw calls in mtmd do? |
github-actions
Bot
added
server
mtmd
|
@ServeurpersoCom Thanks for testing. Tomorrow I'll exercise this branch with my workflows too. |
|
@ngxson Could you rebase or merge master? |
|
Retested after the last commit (My same Opus 5.5 reviewing context) with Gemma-4-31B-IT + mmproj and a Gemma-4-E2B-IT draft (draft-simple): the only thing missing now is this change, the server aborts on the first image request without it. Target embeddings the draft cannot read become zero rows so the draft positions stay contiguous, same output as master : |
| // a draft with a different width (e.g. a smaller model) gets zeros instead, keeping its positions contiguous | ||
| const bool same_width = t.embd.n_rows * t.embd.n_embd == zeros.size(); | ||
| const llama_embd embd = same_width ? t.embd : llama_embd{ zeros.data(), 1, zeros.size() }; | ||
| batch.add_embd(embd, t.pos.data(), t.seq_id, output); |
There was a problem hiding this comment.
Instead of filling zeroes, should we print warning/error? Do we have a use case that goes through the same_width == false path?
There was a problem hiding this comment.
this case happens when we have mtmd input: input image/audio goes through target's encoder then the projected output is used by both draft+target
the most correct way to handle this is to create 2 mtmd contexts: one for target and one for draft. but this is quite costly to maintain so I think we can skip it for now. second-tier solution is to use a padding token instead of all 0, but I haven't yet measure the impact of this method.
so for now, I think a warning here is valid, will add it
There was a problem hiding this comment.
added: 2f651e6
merging once the CI passes
|
@ngxson There is a build error after this PR: https://github.com/ggml-org/llama.cpp/actions/runs/36461258657/job/109060086204#step:3:1139 |
|
Since I have the audio context warm on my agent I took a look: it's a GCC 15 -Wstringop-overflow false positive, the decode_embd_batch constructor now does logits.resize(n_tokens, 0) which goes through _M_fill_append, and once inlined into the qwen3tts/pockettts step_gen with n_tokens = 1 GCC trips on the relocation path; going back to plain logits.resize(n_tokens) like before the PR (same zero init) should fix it. Fix CI follow-up #29607 |
) * adapt common * add common_batch * wip * wip: spec * cont * common_speculative_process * server_batch to use common_batch * rm some stale calls Assisted-by: Claude Fable 5.1 * migrate mtmd * handle imrope, handle return val of add()/add_embd() * add spec zeros vector * add warning on zero fill path
) * adapt common * add common_batch * wip * wip: spec * cont * common_speculative_process * server_batch to use common_batch * rm some stale calls Assisted-by: Claude Fable 5.1 * migrate mtmd * handle imrope, handle return val of add()/add_embd() * add spec zeros vector * add warning on zero fill path
The only thing that broke was common_speculative_process: it takes a common_batch now instead of a llama_batch (ggml-org/llama.cpp#29385). The MTP wrapper now copies the caller's batch into one, so nothing changes on the Rust side. That path is only compiled, not run. llama.cpp also bumped the session file version, so session files saved with older versions won't load anymore. Tested on macOS with the CI commands (clippy, fmt, build, test, publish dry-run).
DFlash2 processes images by injecting target features into the draft KV cache, but an M-RoPE image pins all its rows to one position, so those rows are skipped and the draft cache misses the image position span. The next batch then starts past the draft last stored position and the batch allocator rejects it: E init: the tokens of sequence 0 ... have inconsistent sequence positions: - the last position stored ... X = 12 - the tokens ... have a starting position of Y = 57 it is required that the sequence positions remain consecutive: Y = X + 1 which surfaces as HTTP 500 "decode() failed: failed to process speculative batch" on every vision request (text-only is fine). Fill the missing span with zero rows - the substitution mainline applies to multimodal inputs a draft cannot read (ref: ggml-org/llama.cpp#29385) - both before injecting a batch (process) and before decoding the noise block (draft), so the draft positions stay contiguous. Works for drafts with and without M-RoPE sections and for trailing/multi-image prompts. Fixes #2 Co-Authored-By: Hermes-Tianji <hermes-tianji@users.noreply.github.com>
The only thing that broke was common_speculative_process: it takes a common_batch now instead of a llama_batch (ggml-org/llama.cpp#29385). The MTP wrapper now copies the caller's batch into one, so nothing changes on the Rust side. That path is only compiled, not run. llama.cpp also bumped the session file version, so session files saved with older versions won't load anymore. Tested on macOS with the CI commands (clippy, fmt, build, test, publish dry-run).
3 participants
Overview
Cont #24669
Key point here is
common_batchwhich is a wrapper forllama_batch_ext, providing getter functionsWhile we can add getters to
llama_batch_ext, I feel like it will add too much maintenance in long term. We can add them later if many users request that.Requirements