feat(vision): native mmproj multimodal chat for Qwen35 - #571
Open
davidmroth wants to merge 10 commits into
Open
Conversation
Load mmproj alongside draft, parse image_url in HTTP server, inject vision embeddings in Qwen35 prefill, and keep speculative decode for text-only requests while forcing AR on multimodal turns.
Agent payloads (38+ tools) tokenize into a single large mtmd text chunk. Prefilling it in one graph build reserved ~8.7GB VRAM and failed on dual-GPU deploys. Sub-chunk text segments with DFLASH27B_PREFILL_UBATCH (same as text prefill) and release scratch buffers before multimodal prefill starts.
Wire mmproj through the layer-split adapter and backend so dflash_server can run vision prefill across sharded GPUs with mRoPE/bidirectional masks.
…per. Forward-declare LayerSplitAttnPrefillOpts via layer_split_forward.h and use the shared build_bidirectional_mask from attn_masks.h in qwen35_backend.
GENERATE_MULTIMODAL checked ModelBackend::supports_multimodal(), but LayerSplitBackend never delegated to the adapter, so vision failed with vision_not_configured even after mmproj loaded successfully.
vision_ only exists when mmproj is enabled; ggml-only builds must not reference it from the inline override.
Layer-split and Qwen35 overrides need a base virtual; without it MMPROJ builds fail with 'marked override, but does not override'.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional native mmproj-based multimodal (vision) support to dflash_server for Qwen35 by integrating llama.cpp’s mtmd projector/tokenization path, plumbing OpenAI-style image parts through request parsing, and forcing AR decode on multimodal turns while preserving speculative decode on text-only turns.
Changes:
- Introduces vision payload parsing (base64 + OpenAI-style message content extraction) and a
VisionEncoderwrapper around mtmd. - Plumbs multimodal prompts through the HTTP server → backend request path, with backend/adapter multimodal prefill implementations (monolithic + layer-split).
- Adds build/runtime wiring (CMake option + CLI/env flags) and surfaces
capabilities.vision_supportedvia/props.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/vision/vision_input.h | Declares base64 + message-to-multimodal extraction helpers. |
| server/src/vision/vision_input.cpp | Implements base64/data-URL decode and multimodal message extraction. |
| server/src/vision/vision_encoder.h | Declares mtmd-based vision encoder wrapper (stubbed when disabled). |
| server/src/vision/vision_encoder.cpp | Implements mmproj load + mtmd tokenize/encode wrapper. |
| server/src/server/server_main.cpp | Adds --mmproj / --no-mmproj-offload CLI plumbing and /props vision flag wiring. |
| server/src/server/http_server.h | Extends ServerConfig and ParsedRequest to carry mmproj + multimodal prompt. |
| server/src/server/http_server.cpp | Accepts image parts in message normalization, extracts images, and forces AR decode on multimodal requests; adds /props capability. |
| server/src/qwen35/qwen35_layer_split_vision.cpp | Implements layer-split multimodal prefill (text + image embedding chunks). |
| server/src/qwen35/qwen35_layer_split_adapter.h | Adds multimodal adapter API, mmproj config knobs, and vision encoder member. |
| server/src/qwen35/qwen35_layer_split_adapter.cpp | Initializes/shuts down the vision encoder and exposes current KV position. |
| server/src/qwen35/qwen35_backend.h | Adds mmproj config and backend multimodal capability plumbing. |
| server/src/qwen35/qwen35_backend.cpp | Implements monolithic multimodal prefill and forces AR decode when multimodal. |
| server/src/qwen35/layer_split_forward.h | Adds attention override struct for multimodal prefill (positions/masks). |
| server/src/qwen35/layer_split_forward.cpp | Applies optional multimodal attention overrides (positions + bidirectional masks). |
| server/src/common/vision_types.h | Introduces DecodedImage and MultimodalPrompt shared types. |
| server/src/common/model_backend.h | Extends GenerateRequest with a multimodal prompt and deep-copy support; adds supports_multimodal(). |
| server/src/common/layer_split_backend.h | Adds adapter/backend multimodal APIs and current KV position accessor. |
| server/src/common/layer_split_backend.cpp | Routes multimodal requests through adapter prefill and forces AR decode. |
| server/src/common/backend_factory.cpp | Threads mmproj args into qwen35 backend/adapter configs. |
| server/src/common/backend_args.h | Adds mmproj args to backend argument struct. |
| server/src/common/attn_masks.h | Adds bidirectional attention mask helper for vision chunks. |
| server/scripts/entrypoint.sh | Adds DFLASH_MMPROJ/DFLASH_MMPROJ_NO_OFFLOAD env → CLI wiring. |
| server/docs/VISION.md | Documents build/runtime usage and request shape for vision. |
| server/CMakeLists.txt | Adds optional mtmd/llama build + DFLASH_HAVE_MMPROJ wiring and new sources/includes. |
| .gitattributes | Tracks new docs image via LFS settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+25
to
+28
| // Optional attention overrides for multimodal prefill (mRoPE positions, | ||
| // bidirectional image masks). positions uses layer-split layout: | ||
| // [dim0*n + i, dim1*n + i, dim2*n + i, dim3*n + i] for n query tokens. | ||
| struct LayerSplitAttnPrefillOpts { |
Comment on lines
101
to
+105
| if (req.snap_pos >= 0 && req.snap_slot >= 0 && | ||
| base_pos + consumed == req.snap_pos) { | ||
| if (adapter_->snapshot_save(req.snap_slot)) { | ||
| std::printf("[snap] inline slot=%d cur_pos=%d\n", | ||
| req.snap_slot, req.snap_pos); | ||
| std::fflush(stdout); | ||
| adapter_->snapshot_save(req.snap_slot)) { | ||
| std::printf("[snap] inline slot=%d cur_pos=%d\n", | ||
| req.snap_slot, req.snap_pos); | ||
| std::fflush(stdout); |
Comment on lines
+513
to
+518
| if (attn_opts && attn_opts->positions) { | ||
| const int32_t * src = attn_opts->positions + (size_t)4 * (size_t)start; | ||
| pos_buf.assign(src, src + (size_t)4 * (size_t)n); | ||
| ggml_backend_tensor_set(shard->layer_graph.positions, pos_buf.data(), 0, | ||
| sizeof(int32_t) * pos_buf.size()); | ||
| } else { |
Comment on lines
+9
to
+10
| #include <cctype> | ||
| #include <stdexcept> |
Comment on lines
+11
to
+12
| #include <cstdio> | ||
| #include <vector> |
Comment on lines
+258
to
+262
| } else if (std::strcmp(argv[i], "--mmproj") == 0 && i + 1 < argc) { | ||
| mmproj_path = argv[++i]; | ||
| bargs.mmproj_path = mmproj_path.c_str(); | ||
| sconfig.mmproj_path = mmproj_path; | ||
| sconfig.vision_supported = true; |
| sconfig.fa_window = bargs.fa_window; | ||
| sconfig.ddtree_budget = bargs.ddtree_budget; | ||
| sconfig.speculative_enabled = bargs.ddtree_mode; | ||
| sconfig.vision_supported = !sconfig.mmproj_path.empty(); |
| i = 0; | ||
| } | ||
| } | ||
|
|
Comment on lines
+139
to
+145
| if (header.rfind("data:image/", 0) != 0) { | ||
| throw std::runtime_error("image url must be data:image/...;base64,..."); | ||
| } | ||
| if (header.size() < 7 || header.substr(header.size() - 7) != ";base64") { | ||
| throw std::runtime_error("image url must be base64 encoded"); | ||
| } | ||
| return base64_decode(url.substr(comma + 1)); |
Comment on lines
+148
to
+160
| MultimodalPrompt extract_multimodal_from_messages(const json & messages) { | ||
| MultimodalPrompt mm; | ||
| if (!messages.is_array()) return mm; | ||
|
|
||
| for (const auto & m : messages) { | ||
| if (!m.is_object()) continue; | ||
| if (m.contains("content") && m["content"].is_string()) { | ||
| mm.marked_text += m["content"].get<std::string>(); | ||
| } else if (m.contains("content") && m["content"].is_array()) { | ||
| process_content_array(mm, m["content"]); | ||
| } | ||
| } | ||
| return mm; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
18 issues found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/common/model_backend.h">
<violation number="1" location="server/src/common/model_backend.h:185">
P1: Vision requests can ignore the image on prefix-cache hits because the new multimodal payload is passed through the existing token-only restore path. Multimodal requests should bypass prefix-cache restore/snapshot reuse, or the cache and restore implementation should include the image content and restore multimodal state correctly.</violation>
</file>
<file name="server/src/common/attn_masks.h">
<violation number="1" location="server/src/common/attn_masks.h:92">
P1: Multimodal image chunks lose bidirectional attention when layer-split prefill subdivides them: queries in an earlier sub-batch cannot attend to image tokens in later sub-batches. Process a non-causal image chunk as one batch (or otherwise make all chunk K/V available before applying this mask) instead of limiting the visible range to the current `n_tokens`.</violation>
</file>
<file name="server/CMakeLists.txt">
<violation number="1" location="server/CMakeLists.txt:214">
P1: Enabling `DFLASH27B_MMPROJ` cannot configure from this checkout because the referenced top-level llama.cpp project and `tools/mtmd` are not vendored. Adding the full llama.cpp/mtmd sources (or pointing these calls at a real supplied source tree) is needed before exposing this option.</violation>
<violation number="2" location="server/CMakeLists.txt:214">
P2: When `DFLASH27B_MMPROJ=ON`, this adds the full `deps/llama.cpp` tree as a CMake subdirectory, but the same source is already added a few lines above as `add_subdirectory(deps/llama.cpp/ggml EXCLUDE_FROM_ALL)`. The vendored llama.cpp root CMakeLists normally calls `add_subdirectory(ggml)` unconditionally, so with this option enabled the ggml directory would be configured twice under the same source/binary pair — a CMake hard error at configure time or, if it passes, duplicate `ggml`/`ggml-*` target/symbol definitions when `dflash_server` links the standalone `ggml ${DFLASH27B_GGML_BACKEND_TARGET}` alongside the `llama`/`mtmd` that bundle their own ggml. This is an opt-in build path, but it would make the documented `-DDFLASH27B_MMPROJ=ON` + server build fail rather than being a no-op. Worth confirming the exact llama.cpp snapshot handles this (e.g. gating the standalone ggml add, or renaming targets) before relying on the demo build.</violation>
</file>
<file name="server/src/qwen35/qwen35_layer_split_vision.cpp">
<violation number="1" location="server/src/qwen35/qwen35_layer_split_vision.cpp:163">
P1: Multimodal and multi-token text prefills use incorrect RoPE positions because the layer-split forward API expects dimension-major position storage, while these loops write token-interleaved values. Building the buffers as four contiguous dimension arrays is needed for Qwen35 prompts and image embeddings to receive the intended positional encoding.</violation>
</file>
<file name="server/src/common/backend_factory.cpp">
<violation number="1" location="server/src/common/backend_factory.cpp:247">
P3: Layer-split projector loading treats empty, malformed, or non-positive `DFLASH_MMPROJ_THREADS` as one thread, while monolithic Qwen35 uses the default of four; this can unnecessarily reduce image-encoding throughput on split deployments. Apply the same validated positive-value/default-four parsing in both paths.</violation>
</file>
<file name="server/src/qwen35/qwen35_backend.h">
<violation number="1" location="server/src/qwen35/qwen35_backend.h:283">
P2: Explicit backend shutdown leaves the mmproj context and tokenizer model resident instead of releasing all backend resources. Reset `vision_` in `Qwen35Backend::shutdown()` so repeated or early shutdowns release the native vision allocation.</violation>
</file>
<file name="server/src/server/server_main.cpp">
<violation number="1" location="server/src/server/server_main.cpp:1102">
P1: A `--mmproj` path makes `/props` claim vision support even when the selected backend/build cannot serve it: hybrid `qwen35moe` requests silently run the text-only path, while an MMPROJ-off build fails later as `vision not configured`. Reject the projector at startup unless `backend->supports_multimodal()` is true, or clear the path/capability before constructing `HttpServer`.</violation>
</file>
<file name="server/src/qwen35/layer_split_forward.h">
<violation number="1" location="server/src/qwen35/layer_split_forward.h:30">
P1: Non-causal image prefill becomes causal whenever KVFlash is enabled, despite this new flag. Passing bidirectional mode into slot-space mask construction, or disabling KVFlash for such chunks, would retain full image-token visibility.</violation>
</file>
<file name="server/src/vision/vision_input.cpp">
<violation number="1" location="server/src/vision/vision_input.cpp:74">
P1: Valid OpenAI Responses `input_image` parts with a string `image_url` are rejected, so multimodal requests through `/v1/responses` return 400 even for valid data URLs. Decode string data URLs directly for `input_image`, while retaining object handling for Chat Completions `image_url`.</violation>
<violation number="2" location="server/src/vision/vision_input.cpp:102">
P2: Malformed Base64 is silently truncated instead of rejected, so a corrupted image can be decoded and passed to the vision pipeline. Validate the complete input, padding, and legal length before returning bytes.</violation>
</file>
<file name="server/src/qwen35/qwen35_backend.cpp">
<violation number="1" location="server/src/qwen35/qwen35_backend.cpp:1342">
P1: With `DFLASH_KVFLASH` enabled, multimodal prefill writes logical positions directly into the pool and leaves the previous pager mapping and mask intact, producing incorrect attention or an out-of-bounds cache write. Route multimodal prefill through the pooled `kv_write_rows`/pager path or reject this incompatible combination.</violation>
<violation number="2" location="server/src/qwen35/qwen35_backend.cpp:1523">
P1: Near the configured context limit, an image can make `committed` exceed the allocated KV cache even though HTTP validation accepted the request. Validate expanded mtmd positions against `cfg_.device.max_ctx` before building the chunk and return a prefill error when it would overflow.</violation>
<violation number="3" location="server/src/qwen35/qwen35_backend.cpp:1542">
P1: Greedy multimodal completions start from the argmax of the first prefill position instead of the final prompt position. Read the final argmax row for the final image chunk and track the final text subbatch when text was split.</violation>
<violation number="4" location="server/src/qwen35/qwen35_backend.cpp:1546">
P1: Sampling a multimodal prompt whose final mtmd text chunk exceeds `DFLASH27B_PREFILL_UBATCH` can read logits beyond the final graph buffer. Store the final subbatch size/offset rather than using the full chunk token count.</violation>
</file>
<file name="server/src/server/http_server.cpp">
<violation number="1" location="server/src/server/http_server.cpp:1787">
P2: Image/decode failures raised inside extract_multimodal_from_messages (invalid base64, missing image_url, empty decoded image) propagate to route_request's pre-existing catch-all and are returned to the client as "JSON parse error: <cause>". A base64 or image-URL decode failure is not a JSON parsing error, so this label is misleading for API consumers. Consider catching std::exception around the extraction call in render_and_tokenize_request and sending a 400 with a vision-specific message so the error text matches the actual failure.</violation>
<violation number="2" location="server/src/server/http_server.cpp:3129">
P1: Long multimodal requests can be PFlash-compressed for accounting and cache purposes while the backend still pre-fills the original uncompressed prompt, so the model sees a different context than the server validates and tracks. Skipping PFlash/FlowKV for multimodal turns would keep the prompt and image embedding sequence consistent.</violation>
</file>
<file name="server/src/common/layer_split_backend.cpp">
<violation number="1" location="server/src/common/layer_split_backend.cpp:116">
P2: Multimodal prefix-cache snapshots are saved at the end of the full image prefill instead of at the requested `snap_pos`, so inline cache entries are discarded or can describe more prompt tokens than their saved KV state. Saving only when `req.snap_pos == committed` (or implementing boundary-aware multimodal prefill) keeps cache metadata consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| bool force_ar_decode = false; | ||
| // Native mmproj vision prompt. When set, backends run multimodal prefill | ||
| // instead of token-id prefill and force AR decode. | ||
| std::unique_ptr<MultimodalPrompt> multimodal; |
Contributor
There was a problem hiding this comment.
P1: Vision requests can ignore the image on prefix-cache hits because the new multimodal payload is passed through the existing token-only restore path. Multimodal requests should bypass prefix-cache restore/snapshot reuse, or the cache and restore implementation should include the image content and restore multimodal state correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/model_backend.h, line 185:
<comment>Vision requests can ignore the image on prefix-cache hits because the new multimodal payload is passed through the existing token-only restore path. Multimodal requests should bypass prefix-cache restore/snapshot reuse, or the cache and restore implementation should include the image content and restore multimodal state correctly.</comment>
<file context>
@@ -177,8 +180,58 @@ struct GenerateRequest {
bool force_ar_decode = false;
+ // Native mmproj vision prompt. When set, backends run multimodal prefill
+ // instead of token-id prefill and force AR decode.
+ std::unique_ptr<MultimodalPrompt> multimodal;
+
+ GenerateRequest() = default;
</file context>
| for (int k = 0; k < kv_pos; k++) { | ||
| out[(size_t)q * kv_pad + k] = F16_ZERO; | ||
| } | ||
| for (int k = kv_pos; k < kv_pos + n_tokens; k++) { |
Contributor
There was a problem hiding this comment.
P1: Multimodal image chunks lose bidirectional attention when layer-split prefill subdivides them: queries in an earlier sub-batch cannot attend to image tokens in later sub-batches. Process a non-causal image chunk as one batch (or otherwise make all chunk K/V available before applying this mask) instead of limiting the visible range to the current n_tokens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/attn_masks.h, line 92:
<comment>Multimodal image chunks lose bidirectional attention when layer-split prefill subdivides them: queries in an earlier sub-batch cannot attend to image tokens in later sub-batches. Process a non-causal image chunk as one batch (or otherwise make all chunk K/V available before applying this mask) instead of limiting the visible range to the current `n_tokens`.</comment>
<file context>
@@ -75,4 +75,24 @@ inline void build_tree_mask(const DDTree & tree, int past_length,
+ for (int k = 0; k < kv_pos; k++) {
+ out[(size_t)q * kv_pad + k] = F16_ZERO;
+ }
+ for (int k = kv_pos; k < kv_pos + n_tokens; k++) {
+ out[(size_t)q * kv_pad + k] = F16_ZERO;
+ }
</file context>
| set(LLAMA_BUILD_TOOLS OFF CACHE BOOL "" FORCE) | ||
| set(LLAMA_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE) | ||
| set(LLAMA_BUILD_SERVER OFF CACHE BOOL "" FORCE) | ||
| add_subdirectory(deps/llama.cpp EXCLUDE_FROM_ALL) |
Contributor
There was a problem hiding this comment.
P1: Enabling DFLASH27B_MMPROJ cannot configure from this checkout because the referenced top-level llama.cpp project and tools/mtmd are not vendored. Adding the full llama.cpp/mtmd sources (or pointing these calls at a real supplied source tree) is needed before exposing this option.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/CMakeLists.txt, line 214:
<comment>Enabling `DFLASH27B_MMPROJ` cannot configure from this checkout because the referenced top-level llama.cpp project and `tools/mtmd` are not vendored. Adding the full llama.cpp/mtmd sources (or pointing these calls at a real supplied source tree) is needed before exposing this option.</comment>
<file context>
@@ -203,6 +203,23 @@ endif()
+ set(LLAMA_BUILD_TOOLS OFF CACHE BOOL "" FORCE)
+ set(LLAMA_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE)
+ set(LLAMA_BUILD_SERVER OFF CACHE BOOL "" FORCE)
+ add_subdirectory(deps/llama.cpp EXCLUDE_FROM_ALL)
+ # mtmd is added as a sibling of llama.cpp (not via tools/) so
+ # LLAMA_INSTALL_VERSION must be visible in the parent scope.
</file context>
| std::vector<int32_t> pos_buf((size_t)4 * (size_t)sub_n, 0); | ||
| for (int i = 0; i < sub_n; i++) { | ||
| const int p = kv_pos + start + i; | ||
| pos_buf[4 * i + 0] = p; |
Contributor
There was a problem hiding this comment.
P1: Multimodal and multi-token text prefills use incorrect RoPE positions because the layer-split forward API expects dimension-major position storage, while these loops write token-interleaved values. Building the buffers as four contiguous dimension arrays is needed for Qwen35 prompts and image embeddings to receive the intended positional encoding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_layer_split_vision.cpp, line 163:
<comment>Multimodal and multi-token text prefills use incorrect RoPE positions because the layer-split forward API expects dimension-major position storage, while these loops write token-interleaved values. Building the buffers as four contiguous dimension arrays is needed for Qwen35 prompts and image embeddings to receive the intended positional encoding.</comment>
<file context>
@@ -0,0 +1,247 @@
+ std::vector<int32_t> pos_buf((size_t)4 * (size_t)sub_n, 0);
+ for (int i = 0; i < sub_n; i++) {
+ const int p = kv_pos + start + i;
+ pos_buf[4 * i + 0] = p;
+ pos_buf[4 * i + 1] = p;
+ pos_buf[4 * i + 2] = p;
</file context>
| sconfig.fa_window = bargs.fa_window; | ||
| sconfig.ddtree_budget = bargs.ddtree_budget; | ||
| sconfig.speculative_enabled = bargs.ddtree_mode; | ||
| sconfig.vision_supported = !sconfig.mmproj_path.empty(); |
Contributor
There was a problem hiding this comment.
P1: A --mmproj path makes /props claim vision support even when the selected backend/build cannot serve it: hybrid qwen35moe requests silently run the text-only path, while an MMPROJ-off build fails later as vision not configured. Reject the projector at startup unless backend->supports_multimodal() is true, or clear the path/capability before constructing HttpServer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/server_main.cpp, line 1102:
<comment>A `--mmproj` path makes `/props` claim vision support even when the selected backend/build cannot serve it: hybrid `qwen35moe` requests silently run the text-only path, while an MMPROJ-off build fails later as `vision not configured`. Reject the projector at startup unless `backend->supports_multimodal()` is true, or clear the path/capability before constructing `HttpServer`.</comment>
<file context>
@@ -1081,6 +1099,7 @@ int main(int argc, char ** argv) {
sconfig.fa_window = bargs.fa_window;
sconfig.ddtree_budget = bargs.ddtree_budget;
sconfig.speculative_enabled = bargs.ddtree_mode;
+ sconfig.vision_supported = !sconfig.mmproj_path.empty();
sconfig.target_sharding = bargs.device.is_layer_split();
// KV type: report the operator's choice if set, else the family default
</file context>
Suggested change
| sconfig.vision_supported = !sconfig.mmproj_path.empty(); | |
| if (!sconfig.mmproj_path.empty() && !backend->supports_multimodal()) { | |
| std::fprintf(stderr, "[server] --mmproj is unsupported by this backend/build\n"); | |
| backend->shutdown(); | |
| return 2; | |
| } | |
| sconfig.vision_supported = !sconfig.mmproj_path.empty(); |
| std::vector<uint8_t> ret; | ||
| ret.reserve((size_t)in_len * 3 / 4); | ||
|
|
||
| while (in_len - in_ > 0 && encoded_string[in_] != '=' && is_base64(encoded_string[in_])) { |
Contributor
There was a problem hiding this comment.
P2: Malformed Base64 is silently truncated instead of rejected, so a corrupted image can be decoded and passed to the vision pipeline. Validate the complete input, padding, and legal length before returning bytes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/vision/vision_input.cpp, line 102:
<comment>Malformed Base64 is silently truncated instead of rejected, so a corrupted image can be decoded and passed to the vision pipeline. Validate the complete input, padding, and legal length before returning bytes.</comment>
<file context>
@@ -0,0 +1,176 @@
+ std::vector<uint8_t> ret;
+ ret.reserve((size_t)in_len * 3 / 4);
+
+ while (in_len - in_ > 0 && encoded_string[in_] != '=' && is_base64(encoded_string[in_])) {
+ char_array_4[i++] = (unsigned char)encoded_string[in_++];
+ if (i == 4) {
</file context>
| if (adapter_chunk > 0 && n_tokens > adapter_chunk) { | ||
| n_tokens = adapter_chunk; | ||
| } | ||
| if (req.snap_pos >= 0 && req.snap_slot >= 0 && |
Contributor
There was a problem hiding this comment.
P2: Multimodal prefix-cache snapshots are saved at the end of the full image prefill instead of at the requested snap_pos, so inline cache entries are discarded or can describe more prompt tokens than their saved KV state. Saving only when req.snap_pos == committed (or implementing boundary-aware multimodal prefill) keeps cache metadata consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/layer_split_backend.cpp, line 116:
<comment>Multimodal prefix-cache snapshots are saved at the end of the full image prefill instead of at the requested `snap_pos`, so inline cache entries are discarded or can describe more prompt tokens than their saved KV state. Saving only when `req.snap_pos == committed` (or implementing boundary-aware multimodal prefill) keeps cache metadata consistent.</comment>
<file context>
@@ -69,39 +77,65 @@ GenerateResult LayerSplitBackend::run_from_state(const GenerateRequest & req,
+ if (adapter_chunk > 0 && n_tokens > adapter_chunk) {
+ n_tokens = adapter_chunk;
+ }
+ if (req.snap_pos >= 0 && req.snap_slot >= 0 &&
+ req.snap_pos > base_pos + consumed &&
+ req.snap_pos < base_pos + consumed + n_tokens) {
</file context>
| set(LLAMA_BUILD_TOOLS OFF CACHE BOOL "" FORCE) | ||
| set(LLAMA_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE) | ||
| set(LLAMA_BUILD_SERVER OFF CACHE BOOL "" FORCE) | ||
| add_subdirectory(deps/llama.cpp EXCLUDE_FROM_ALL) |
Contributor
There was a problem hiding this comment.
P2: When DFLASH27B_MMPROJ=ON, this adds the full deps/llama.cpp tree as a CMake subdirectory, but the same source is already added a few lines above as add_subdirectory(deps/llama.cpp/ggml EXCLUDE_FROM_ALL). The vendored llama.cpp root CMakeLists normally calls add_subdirectory(ggml) unconditionally, so with this option enabled the ggml directory would be configured twice under the same source/binary pair — a CMake hard error at configure time or, if it passes, duplicate ggml/ggml-* target/symbol definitions when dflash_server links the standalone ggml ${DFLASH27B_GGML_BACKEND_TARGET} alongside the llama/mtmd that bundle their own ggml. This is an opt-in build path, but it would make the documented -DDFLASH27B_MMPROJ=ON + server build fail rather than being a no-op. Worth confirming the exact llama.cpp snapshot handles this (e.g. gating the standalone ggml add, or renaming targets) before relying on the demo build.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/CMakeLists.txt, line 214:
<comment>When `DFLASH27B_MMPROJ=ON`, this adds the full `deps/llama.cpp` tree as a CMake subdirectory, but the same source is already added a few lines above as `add_subdirectory(deps/llama.cpp/ggml EXCLUDE_FROM_ALL)`. The vendored llama.cpp root CMakeLists normally calls `add_subdirectory(ggml)` unconditionally, so with this option enabled the ggml directory would be configured twice under the same source/binary pair — a CMake hard error at configure time or, if it passes, duplicate `ggml`/`ggml-*` target/symbol definitions when `dflash_server` links the standalone `ggml ${DFLASH27B_GGML_BACKEND_TARGET}` alongside the `llama`/`mtmd` that bundle their own ggml. This is an opt-in build path, but it would make the documented `-DDFLASH27B_MMPROJ=ON` + server build fail rather than being a no-op. Worth confirming the exact llama.cpp snapshot handles this (e.g. gating the standalone ggml add, or renaming targets) before relying on the demo build.</comment>
<file context>
@@ -203,6 +203,23 @@ endif()
+ set(LLAMA_BUILD_TOOLS OFF CACHE BOOL "" FORCE)
+ set(LLAMA_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE)
+ set(LLAMA_BUILD_SERVER OFF CACHE BOOL "" FORCE)
+ add_subdirectory(deps/llama.cpp EXCLUDE_FROM_ALL)
+ # mtmd is added as a sibling of llama.cpp (not via tools/) so
+ # LLAMA_INSTALL_VERSION must be visible in the parent scope.
</file context>
| "vision input requires --mmproj (no mmproj model loaded)"); | ||
| return false; | ||
| } | ||
| MultimodalPrompt extracted = extract_multimodal_from_messages(req.messages); |
Contributor
There was a problem hiding this comment.
P2: Image/decode failures raised inside extract_multimodal_from_messages (invalid base64, missing image_url, empty decoded image) propagate to route_request's pre-existing catch-all and are returned to the client as "JSON parse error: ". A base64 or image-URL decode failure is not a JSON parsing error, so this label is misleading for API consumers. Consider catching std::exception around the extraction call in render_and_tokenize_request and sending a 400 with a vision-specific message so the error text matches the actual failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/http_server.cpp, line 1787:
<comment>Image/decode failures raised inside extract_multimodal_from_messages (invalid base64, missing image_url, empty decoded image) propagate to route_request's pre-existing catch-all and are returned to the client as "JSON parse error: <cause>". A base64 or image-URL decode failure is not a JSON parsing error, so this label is misleading for API consumers. Consider catching std::exception around the extraction call in render_and_tokenize_request and sending a 400 with a vision-specific message so the error text matches the actual failure.</comment>
<file context>
@@ -1764,6 +1778,22 @@ bool HttpServer::render_and_tokenize_request(
+ "vision input requires --mmproj (no mmproj model loaded)");
+ return false;
+ }
+ MultimodalPrompt extracted = extract_multimodal_from_messages(req.messages);
+ if (extracted.images.empty()) {
+ send_error(fd, 400, "failed to decode vision input");
</file context>
Comment on lines
+247
to
+249
| if (const char * mt = std::getenv("DFLASH_MMPROJ_THREADS")) { | ||
| cfg.mmproj_threads = std::max(1, std::atoi(mt)); | ||
| } |
Contributor
There was a problem hiding this comment.
P3: Layer-split projector loading treats empty, malformed, or non-positive DFLASH_MMPROJ_THREADS as one thread, while monolithic Qwen35 uses the default of four; this can unnecessarily reduce image-encoding throughput on split deployments. Apply the same validated positive-value/default-four parsing in both paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/backend_factory.cpp, line 247:
<comment>Layer-split projector loading treats empty, malformed, or non-positive `DFLASH_MMPROJ_THREADS` as one thread, while monolithic Qwen35 uses the default of four; this can unnecessarily reduce image-encoding throughput on split deployments. Apply the same validated positive-value/default-four parsing in both paths.</comment>
<file context>
@@ -242,6 +242,11 @@ std::unique_ptr<ModelBackend> create_backend(
cfg.run_dflash = args.draft_path != nullptr;
+ cfg.mmproj_path = args.mmproj_path;
+ cfg.mmproj_use_gpu = args.mmproj_use_gpu;
+ if (const char * mt = std::getenv("DFLASH_MMPROJ_THREADS")) {
+ cfg.mmproj_threads = std::max(1, std::atoi(mt));
+ }
</file context>
Suggested change
| if (const char * mt = std::getenv("DFLASH_MMPROJ_THREADS")) { | |
| cfg.mmproj_threads = std::max(1, std::atoi(mt)); | |
| } | |
| if (const char * mt = std::getenv("DFLASH_MMPROJ_THREADS"); | |
| mt && *mt) { | |
| char * end = nullptr; | |
| const long parsed = std::strtol(mt, &end, 10); | |
| if (end != mt && *end == '\0' && parsed > 0) { | |
| cfg.mmproj_threads = static_cast<int>(parsed); | |
| } | |
| } |
Contributor
There was a problem hiding this comment.
23 issues found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/CMakeLists.txt">
<violation number="1" location="server/CMakeLists.txt:214">
P1: The advertised mmproj build cannot configure because the vendored `deps/llama.cpp` tree has neither the top-level CMake project nor the `tools/mtmd` subtree referenced here. Vendor the full llama.cpp/mtmd sources or point these calls at a checked-in mtmd integration before enabling this option.</violation>
</file>
<file name="server/src/common/layer_split_backend.cpp">
<violation number="1" location="server/src/common/layer_split_backend.cpp:92">
P1: An image whose expanded mtmd token count exceeds the context limit is not rejected safely: multimodal prefill writes/builds KV views first and checks capacity only afterward, risking a backend failure or crash instead of a clean `ContextOverflow`. Preflight the expanded chunk positions or enforce the bound inside multimodal prefill before building each forward graph.</violation>
<violation number="2" location="server/src/common/layer_split_backend.cpp:92">
P1: Multimodal requests that hit the prefix cache generate from corrupted state: the restored KV prefix is overwritten by a full mmproj prefill starting at position zero. Multimodal restore should be rejected or routed through a fresh full prefill, as the monolithic backend already does for nonzero restore offsets.</violation>
<violation number="3" location="server/src/common/layer_split_backend.cpp:116">
P2: Multimodal inline snapshots can capture the end of the expanded image prefill at a position different from `req.snap_pos`, so the saved state does not represent the requested prefix boundary and can poison later prefix-cache restores. Save only when `committed == req.snap_pos`.</violation>
</file>
<file name="server/src/server/server_main.cpp">
<violation number="1" location="server/src/server/server_main.cpp:1102">
P1: `/props` advertises vision for configurations whose constructed backend has no multimodal support, so clients can send image requests that fail or run without image handling. Please reject `--mmproj` when the optional support/backend is unavailable, or derive this capability from `backend->supports_multimodal()` and reject image input when it is false.</violation>
</file>
<file name="server/src/common/attn_masks.h">
<violation number="1" location="server/src/common/attn_masks.h:89">
P1: Layer-split multimodal prefill can attend to padded KV rows when `--fa-window` is combined with a padded FA stride. Building this mask relative to `win_start` and the valid window span, or disabling windowed multimodal prefill, avoids unmasking invalid cache rows.</violation>
</file>
<file name="server/src/vision/vision_input.cpp">
<violation number="1" location="server/src/vision/vision_input.cpp:74">
P2: Standard Responses `input_image` parts with a string `image_url` are rejected even when the data URL is valid. Accept the string form before the existing object form so `/v1/responses` vision requests reach the encoder.</violation>
<violation number="2" location="server/src/vision/vision_input.cpp:79">
P2: Anthropic `image` content blocks fail to decode. The new part_is_image_type() treats Anthropic's `type:"image"` as an image (messages_contain_images and normalize_chat_messages both accept it and insert a marker), but process_content_array() routes non-`input_image` parts to decode_image_url_object(), which only understands `image_url`. Anthropic image blocks carry `source:{type:"base64",data:...}` with no `image_url`, so the call throws "image_url.url is required" and the request is rejected with a 400 despite mmproj being loaded. If Anthropic vision is intended, decode the `source.base64` payload the same way `input_image` is handled; otherwise exclude `image`/Anthropic blocks from messages_contain_images / normalization so the marker count and image count stay consistent.</violation>
<violation number="3" location="server/src/vision/vision_input.cpp:102">
P2: Malformed base64 is silently truncated instead of rejected, so a request with a valid image prefix followed by junk can be queued as if it were valid and fail later in image decoding. Validate the complete input, padding, and trailing characters before returning decoded bytes.</violation>
</file>
<file name="server/src/server/http_server.cpp">
<violation number="1" location="server/src/server/http_server.cpp:3128">
P1: With PFlash enabled, multimodal prefill ignores the compressed prompt, so a long image request can pass the compressed context check but still run the original over-limit history. Multimodal requests should skip PFlash, or regenerate the marked multimodal text and all associated positions from the compressed representation.</violation>
<violation number="2" location="server/src/server/http_server.cpp:3129">
P1: A multimodal request can restore KV state generated for a different image, producing an answer about the wrong image (or losing the image on a partial restore). Multimodal requests should bypass token-only prefix/full/disk caches, or include a stable digest of every image in the cache key.</violation>
</file>
<file name="server/src/vision/vision_encoder.cpp">
<violation number="1" location="server/src/vision/vision_encoder.cpp:31">
P1: Enabling vision loads the entire target GGUF again on CPU instead of a vocab-only model, which can add tens of GB of host mapping/page pressure and make 27B startup fail or thrash. Set `mparams.vocab_only = true` before loading the tokenizer model.</violation>
</file>
<file name="server/src/qwen35/layer_split_forward.cpp">
<violation number="1" location="server/src/qwen35/layer_split_forward.cpp:539">
P1: Non-causal image chunks become causal whenever KVFlash is enabled, so image tokens cannot attend to later image tokens as required by the projector path. The KVFlash mask path should accept and apply the bidirectional option, or multimodal KVFlash prefill should be rejected explicitly.</violation>
<violation number="2" location="server/src/qwen35/layer_split_forward.cpp:540">
P1: Images larger than the configured prefill ubatch do not receive full bidirectional attention: early image tokens cannot see later image tokens. Non-causal image chunks should be processed in one attention batch or use a staging strategy that makes the entire chunk visible before computing its queries.</violation>
<violation number="3" location="server/src/qwen35/layer_split_forward.cpp:540">
P1: With `fa_window > 0` and a multimodal chunk beyond the window, the bidirectional mask is offset from the K/V view: prior-window entries remain visible but the current image tokens are masked. Pass window-relative positions to the helper or force full attention for this prefill path.</violation>
</file>
<file name="server/src/qwen35/qwen35_layer_split_vision.cpp">
<violation number="1" location="server/src/qwen35/qwen35_layer_split_vision.cpp:44">
P1: Remote target-shard deployments advertise vision support but reject every multimodal prefill with `mixed target split unsupported`, so `/props` and request behavior disagree. Either report multimodal support as false for mixed mode or add activation forwarding for the remote shard path.</violation>
<violation number="2" location="server/src/qwen35/qwen35_layer_split_vision.cpp:200">
P1: The multimodal M-RoPE positions buffer is laid out transposed relative to what ggml_rope_multi expects, so the rotary embeddings for image tokens (and the text/audio tokens in the same prefill) will be decoded from the wrong positions, corrupting multimodal output.
The ggml CPU kernel (server/deps/llama.cpp/ggml/src/ggml-cpu/ops.cpp) reads positions as pos[i2], pos[i2+ne2], pos[i2+ne2*2], pos[i2+ne2*3] (index = token + n_positions*section), and the CUDA kernel (ggml-cuda/rope.cu, pos[i2 + ne02*sec]) agrees — i.e. the tensor is column-major (all tokens' 't', then all 'h', then all 'w', then 'e'). The existing default layer-split text path in the same PR file (layer_split_forward.cpp) already writes this correctly as pos_buf[0*n+i]=p, [1*n+i]=p, [2*n+i]=p, [3*n+i]=0, and the added LayerSplitAttnPrefillOpts comment documents the same layout: "[dim0*n + i, dim1*n + i, dim2*n + i, dim3*n + i]".
However, this new vision code (text-chunk lines 163-166, M-RoPE image lines 200-206, and non-M-RoPE image lines 213-216) writes a row-major (token-major) buffer pos_buf[4*i + section]. With the kernels reading pos[i + n*section], each token's four section values resolve to what was stored for a different (i, section) pair, scrambling the mRoPE t/h/w positions. Text-only inference is unaffected only because the non-multimodal path never uses this buffer (it uses the default column-major branch in layer_split_forward.cpp).</violation>
<violation number="3" location="server/src/qwen35/qwen35_layer_split_vision.cpp:222">
P1: Oversized multimodal requests can be forwarded past the target cache's logical context before the server reports `ContextOverflow`, risking a failed graph or out-of-range cache access. Validate each chunk's logical end (`n_tokens` for text and `n_pos` for an image/audio chunk) against `cfg_.device.max_ctx` before encoding or forwarding it.</violation>
</file>
<file name="server/src/qwen35/qwen35_backend.cpp">
<violation number="1" location="server/src/qwen35/qwen35_backend.cpp:865">
P1: Restored multimodal requests ignore their image content: this flag forces AR decode even though the restore path never calls `do_prefill_multimodal`. Route image restores through a fresh multimodal prefill or reject prefix-cache restore for multimodal requests.</violation>
<violation number="2" location="server/src/qwen35/qwen35_backend.cpp:1342">
P1: Multimodal prefill is incompatible with active kvflash: it writes physical positions without updating the pager, then AR decode consumes that pager state. Add the same pooled slot/mask/history synchronization as text prefill or reject multimodal requests when kvflash is enabled.</violation>
<violation number="3" location="server/src/qwen35/qwen35_backend.cpp:1523">
P1: An image request can pass context admission but overflow the target context after projector expansion, causing late prefill failure or out-of-bounds cache access. Check expanded multimodal length against `cfg_.device.max_ctx` and the generation budget before running the graph.</violation>
<violation number="4" location="server/src/qwen35/qwen35_backend.cpp:1542">
P1: Multimodal generation starts from the first row of the final prefill chunk rather than the final prompt row, producing an incorrect first token for greedy/AR decoding. Track the final subchunk row and read its last argmax result, matching `do_prefill`.</violation>
<violation number="5" location="server/src/qwen35/qwen35_backend.cpp:1545">
P1: Sampled multimodal requests can read invalid final logits when the final text chunk is split: the stored offset uses the full chunk length instead of the final subchunk length. Store the offset from the final subchunk's last row.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| set(LLAMA_BUILD_TOOLS OFF CACHE BOOL "" FORCE) | ||
| set(LLAMA_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE) | ||
| set(LLAMA_BUILD_SERVER OFF CACHE BOOL "" FORCE) | ||
| add_subdirectory(deps/llama.cpp EXCLUDE_FROM_ALL) |
Contributor
There was a problem hiding this comment.
P1: The advertised mmproj build cannot configure because the vendored deps/llama.cpp tree has neither the top-level CMake project nor the tools/mtmd subtree referenced here. Vendor the full llama.cpp/mtmd sources or point these calls at a checked-in mtmd integration before enabling this option.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/CMakeLists.txt, line 214:
<comment>The advertised mmproj build cannot configure because the vendored `deps/llama.cpp` tree has neither the top-level CMake project nor the `tools/mtmd` subtree referenced here. Vendor the full llama.cpp/mtmd sources or point these calls at a checked-in mtmd integration before enabling this option.</comment>
<file context>
@@ -203,6 +203,23 @@ endif()
+ set(LLAMA_BUILD_TOOLS OFF CACHE BOOL "" FORCE)
+ set(LLAMA_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE)
+ set(LLAMA_BUILD_SERVER OFF CACHE BOOL "" FORCE)
+ add_subdirectory(deps/llama.cpp EXCLUDE_FROM_ALL)
+ # mtmd is added as a sibling of llama.cpp (not via tools/) so
+ # LLAMA_INSTALL_VERSION must be visible in the parent scope.
</file context>
| req.prompt.begin() + consumed + n_tokens); | ||
| if (!adapter_->prefill(chunk, base_pos + consumed, last_tok)) { | ||
| MultimodalPrompt mm = *req.multimodal; | ||
| const int committed = adapter_->prefill_multimodal(mm, last_tok); |
Contributor
There was a problem hiding this comment.
P1: An image whose expanded mtmd token count exceeds the context limit is not rejected safely: multimodal prefill writes/builds KV views first and checks capacity only afterward, risking a backend failure or crash instead of a clean ContextOverflow. Preflight the expanded chunk positions or enforce the bound inside multimodal prefill before building each forward graph.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/layer_split_backend.cpp, line 92:
<comment>An image whose expanded mtmd token count exceeds the context limit is not rejected safely: multimodal prefill writes/builds KV views first and checks capacity only afterward, risking a backend failure or crash instead of a clean `ContextOverflow`. Preflight the expanded chunk positions or enforce the bound inside multimodal prefill before building each forward graph.</comment>
<file context>
@@ -69,39 +77,65 @@ GenerateResult LayerSplitBackend::run_from_state(const GenerateRequest & req,
- req.prompt.begin() + consumed + n_tokens);
- if (!adapter_->prefill(chunk, base_pos + consumed, last_tok)) {
+ MultimodalPrompt mm = *req.multimodal;
+ const int committed = adapter_->prefill_multimodal(mm, last_tok);
+ if (committed < 0) {
result.fail(GenerateErrorCode::PrefillFailed);
</file context>
| req.prompt.begin() + consumed + n_tokens); | ||
| if (!adapter_->prefill(chunk, base_pos + consumed, last_tok)) { | ||
| MultimodalPrompt mm = *req.multimodal; | ||
| const int committed = adapter_->prefill_multimodal(mm, last_tok); |
Contributor
There was a problem hiding this comment.
P1: Multimodal requests that hit the prefix cache generate from corrupted state: the restored KV prefix is overwritten by a full mmproj prefill starting at position zero. Multimodal restore should be rejected or routed through a fresh full prefill, as the monolithic backend already does for nonzero restore offsets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/layer_split_backend.cpp, line 92:
<comment>Multimodal requests that hit the prefix cache generate from corrupted state: the restored KV prefix is overwritten by a full mmproj prefill starting at position zero. Multimodal restore should be rejected or routed through a fresh full prefill, as the monolithic backend already does for nonzero restore offsets.</comment>
<file context>
@@ -69,39 +77,65 @@ GenerateResult LayerSplitBackend::run_from_state(const GenerateRequest & req,
- req.prompt.begin() + consumed + n_tokens);
- if (!adapter_->prefill(chunk, base_pos + consumed, last_tok)) {
+ MultimodalPrompt mm = *req.multimodal;
+ const int committed = adapter_->prefill_multimodal(mm, last_tok);
+ if (committed < 0) {
result.fail(GenerateErrorCode::PrefillFailed);
</file context>
| sconfig.fa_window = bargs.fa_window; | ||
| sconfig.ddtree_budget = bargs.ddtree_budget; | ||
| sconfig.speculative_enabled = bargs.ddtree_mode; | ||
| sconfig.vision_supported = !sconfig.mmproj_path.empty(); |
Contributor
There was a problem hiding this comment.
P1: /props advertises vision for configurations whose constructed backend has no multimodal support, so clients can send image requests that fail or run without image handling. Please reject --mmproj when the optional support/backend is unavailable, or derive this capability from backend->supports_multimodal() and reject image input when it is false.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/server_main.cpp, line 1102:
<comment>`/props` advertises vision for configurations whose constructed backend has no multimodal support, so clients can send image requests that fail or run without image handling. Please reject `--mmproj` when the optional support/backend is unavailable, or derive this capability from `backend->supports_multimodal()` and reject image input when it is false.</comment>
<file context>
@@ -1081,6 +1099,7 @@ int main(int argc, char ** argv) {
sconfig.fa_window = bargs.fa_window;
sconfig.ddtree_budget = bargs.ddtree_budget;
sconfig.speculative_enabled = bargs.ddtree_mode;
+ sconfig.vision_supported = !sconfig.mmproj_path.empty();
sconfig.target_sharding = bargs.device.is_layer_split();
// KV type: report the operator's choice if set, else the family default
</file context>
| const int q_pad = align_up(n_tokens, KQ_MASK_PAD); | ||
| out.assign((size_t)kv_pad * q_pad, F16_NEG_INF); | ||
| for (int q = 0; q < n_tokens; q++) { | ||
| for (int k = 0; k < kv_pos; k++) { |
Contributor
There was a problem hiding this comment.
P1: Layer-split multimodal prefill can attend to padded KV rows when --fa-window is combined with a padded FA stride. Building this mask relative to win_start and the valid window span, or disabling windowed multimodal prefill, avoids unmasking invalid cache rows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/attn_masks.h, line 89:
<comment>Layer-split multimodal prefill can attend to padded KV rows when `--fa-window` is combined with a padded FA stride. Building this mask relative to `win_start` and the valid window span, or disabling windowed multimodal prefill, avoids unmasking invalid cache rows.</comment>
<file context>
@@ -75,4 +75,24 @@ inline void build_tree_mask(const DDTree & tree, int past_length,
+ const int q_pad = align_up(n_tokens, KQ_MASK_PAD);
+ out.assign((size_t)kv_pad * q_pad, F16_NEG_INF);
+ for (int q = 0; q < n_tokens; q++) {
+ for (int k = 0; k < kv_pos; k++) {
+ out[(size_t)q * kv_pad + k] = F16_ZERO;
+ }
</file context>
| (size_t)n_tokens); | ||
| pos_buf.resize((size_t)4 * (size_t)n_tokens, 0); | ||
| for (int i = 0; i < n_tokens; i++) { | ||
| pos_buf[4 * i + 0] = |
Contributor
There was a problem hiding this comment.
P1: The multimodal M-RoPE positions buffer is laid out transposed relative to what ggml_rope_multi expects, so the rotary embeddings for image tokens (and the text/audio tokens in the same prefill) will be decoded from the wrong positions, corrupting multimodal output.
The ggml CPU kernel (server/deps/llama.cpp/ggml/src/ggml-cpu/ops.cpp) reads positions as pos[i2], pos[i2+ne2], pos[i2+ne22], pos[i2+ne23] (index = token + n_positionssection), and the CUDA kernel (ggml-cuda/rope.cu, pos[i2 + ne02sec]) agrees — i.e. the tensor is column-major (all tokens' 't', then all 'h', then all 'w', then 'e'). The existing default layer-split text path in the same PR file (layer_split_forward.cpp) already writes this correctly as pos_buf[0n+i]=p, [1n+i]=p, [2n+i]=p, [3n+i]=0, and the added LayerSplitAttnPrefillOpts comment documents the same layout: "[dim0n + i, dim1n + i, dim2n + i, dim3n + i]".
However, this new vision code (text-chunk lines 163-166, M-RoPE image lines 200-206, and non-M-RoPE image lines 213-216) writes a row-major (token-major) buffer pos_buf[4i + section]. With the kernels reading pos[i + nsection], each token's four section values resolve to what was stored for a different (i, section) pair, scrambling the mRoPE t/h/w positions. Text-only inference is unaffected only because the non-multimodal path never uses this buffer (it uses the default column-major branch in layer_split_forward.cpp).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_layer_split_vision.cpp, line 200:
<comment>The multimodal M-RoPE positions buffer is laid out transposed relative to what ggml_rope_multi expects, so the rotary embeddings for image tokens (and the text/audio tokens in the same prefill) will be decoded from the wrong positions, corrupting multimodal output.
The ggml CPU kernel (server/deps/llama.cpp/ggml/src/ggml-cpu/ops.cpp) reads positions as pos[i2], pos[i2+ne2], pos[i2+ne2*2], pos[i2+ne2*3] (index = token + n_positions*section), and the CUDA kernel (ggml-cuda/rope.cu, pos[i2 + ne02*sec]) agrees — i.e. the tensor is column-major (all tokens' 't', then all 'h', then all 'w', then 'e'). The existing default layer-split text path in the same PR file (layer_split_forward.cpp) already writes this correctly as pos_buf[0*n+i]=p, [1*n+i]=p, [2*n+i]=p, [3*n+i]=0, and the added LayerSplitAttnPrefillOpts comment documents the same layout: "[dim0*n + i, dim1*n + i, dim2*n + i, dim3*n + i]".
However, this new vision code (text-chunk lines 163-166, M-RoPE image lines 200-206, and non-M-RoPE image lines 213-216) writes a row-major (token-major) buffer pos_buf[4*i + section]. With the kernels reading pos[i + n*section], each token's four section values resolve to what was stored for a different (i, section) pair, scrambling the mRoPE t/h/w positions. Text-only inference is unaffected only because the non-multimodal path never uses this buffer (it uses the default column-major branch in layer_split_forward.cpp).</comment>
<file context>
@@ -0,0 +1,247 @@
+ (size_t)n_tokens);
+ pos_buf.resize((size_t)4 * (size_t)n_tokens, 0);
+ for (int i = 0; i < n_tokens; i++) {
+ pos_buf[4 * i + 0] =
+ kv_pos + (int)rel_pos[(size_t)i].t;
+ pos_buf[4 * i + 1] =
</file context>
| if (adapter_chunk > 0 && n_tokens > adapter_chunk) { | ||
| n_tokens = adapter_chunk; | ||
| } | ||
| if (req.snap_pos >= 0 && req.snap_slot >= 0 && |
Contributor
There was a problem hiding this comment.
P2: Multimodal inline snapshots can capture the end of the expanded image prefill at a position different from req.snap_pos, so the saved state does not represent the requested prefix boundary and can poison later prefix-cache restores. Save only when committed == req.snap_pos.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/layer_split_backend.cpp, line 116:
<comment>Multimodal inline snapshots can capture the end of the expanded image prefill at a position different from `req.snap_pos`, so the saved state does not represent the requested prefix boundary and can poison later prefix-cache restores. Save only when `committed == req.snap_pos`.</comment>
<file context>
@@ -69,39 +77,65 @@ GenerateResult LayerSplitBackend::run_from_state(const GenerateRequest & req,
+ if (adapter_chunk > 0 && n_tokens > adapter_chunk) {
+ n_tokens = adapter_chunk;
+ }
+ if (req.snap_pos >= 0 && req.snap_slot >= 0 &&
+ req.snap_pos > base_pos + consumed &&
+ req.snap_pos < base_pos + consumed + n_tokens) {
</file context>
| std::vector<uint8_t> ret; | ||
| ret.reserve((size_t)in_len * 3 / 4); | ||
|
|
||
| while (in_len - in_ > 0 && encoded_string[in_] != '=' && is_base64(encoded_string[in_])) { |
Contributor
There was a problem hiding this comment.
P2: Malformed base64 is silently truncated instead of rejected, so a request with a valid image prefix followed by junk can be queued as if it were valid and fail later in image decoding. Validate the complete input, padding, and trailing characters before returning decoded bytes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/vision/vision_input.cpp, line 102:
<comment>Malformed base64 is silently truncated instead of rejected, so a request with a valid image prefix followed by junk can be queued as if it were valid and fail later in image decoding. Validate the complete input, padding, and trailing characters before returning decoded bytes.</comment>
<file context>
@@ -0,0 +1,176 @@
+ std::vector<uint8_t> ret;
+ ret.reserve((size_t)in_len * 3 / 4);
+
+ while (in_len - in_ > 0 && encoded_string[in_] != '=' && is_base64(encoded_string[in_])) {
+ char_array_4[i++] = (unsigned char)encoded_string[in_++];
+ if (i == 4) {
</file context>
| } | ||
| img.bytes = base64_decode(data); | ||
| } else if (part.contains("image_url")) { | ||
| img.bytes = decode_image_url_object(part["image_url"]); |
Contributor
There was a problem hiding this comment.
P2: Standard Responses input_image parts with a string image_url are rejected even when the data URL is valid. Accept the string form before the existing object form so /v1/responses vision requests reach the encoder.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/vision/vision_input.cpp, line 74:
<comment>Standard Responses `input_image` parts with a string `image_url` are rejected even when the data URL is valid. Accept the string form before the existing object form so `/v1/responses` vision requests reach the encoder.</comment>
<file context>
@@ -0,0 +1,176 @@
+ }
+ img.bytes = base64_decode(data);
+ } else if (part.contains("image_url")) {
+ img.bytes = decode_image_url_object(part["image_url"]);
+ } else {
+ throw std::runtime_error("unsupported input_image source");
</file context>
| throw std::runtime_error("unsupported input_image source"); | ||
| } | ||
| } else { | ||
| img.bytes = decode_image_url_object(part.value("image_url", json::object())); |
Contributor
There was a problem hiding this comment.
P2: Anthropic image content blocks fail to decode. The new part_is_image_type() treats Anthropic's type:"image" as an image (messages_contain_images and normalize_chat_messages both accept it and insert a marker), but process_content_array() routes non-input_image parts to decode_image_url_object(), which only understands image_url. Anthropic image blocks carry source:{type:"base64",data:...} with no image_url, so the call throws "image_url.url is required" and the request is rejected with a 400 despite mmproj being loaded. If Anthropic vision is intended, decode the source.base64 payload the same way input_image is handled; otherwise exclude image/Anthropic blocks from messages_contain_images / normalization so the marker count and image count stay consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/vision/vision_input.cpp, line 79:
<comment>Anthropic `image` content blocks fail to decode. The new part_is_image_type() treats Anthropic's `type:"image"` as an image (messages_contain_images and normalize_chat_messages both accept it and insert a marker), but process_content_array() routes non-`input_image` parts to decode_image_url_object(), which only understands `image_url`. Anthropic image blocks carry `source:{type:"base64",data:...}` with no `image_url`, so the call throws "image_url.url is required" and the request is rejected with a 400 despite mmproj being loaded. If Anthropic vision is intended, decode the `source.base64` payload the same way `input_image` is handled; otherwise exclude `image`/Anthropic blocks from messages_contain_images / normalization so the marker count and image count stay consistent.</comment>
<file context>
@@ -0,0 +1,176 @@
+ throw std::runtime_error("unsupported input_image source");
+ }
+ } else {
+ img.bytes = decode_image_url_object(part.value("image_url", json::object()));
+ }
+ if (img.bytes.empty()) {
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds optional native mmproj vision to
dflash_servervia llama.cpp mtmd:--mmproj/DFLASH_MMPROJimage_urlcontent in chat completionscapabilities.vision_supportedon/propswhen the projector is loadedBuild is opt-in (
-DDFLASH27B_MMPROJ=ON, default OFF) so ggml-only / text-only deploys are unchanged.Docs:
server/docs/VISION.mdQuick start
Need: CUDA GPU, a Qwen3.5/3.6 GGUF + matching
mmproj-F16.gguf, and a fulllucebox-ggmltree. Hub only vendors the ggml subset —tools/mtmdis not in-tree — so a stock configure with-DDFLASH27B_MMPROJ=ONwill fail until mtmd sources are present.After the build you can restore the slim vendor so the tree stays pullable:
Smoke
Also check a plain text turn still works (and still uses DFlash when a draft is configured). Without
--mmproj, image requests should 400 cleanly.Proof
Live multimodal completion against a Qwen3.6 + mmproj build (status chrome scrubbed). The model reads the attached meme and answers in natural language:
Test plan
DFLASH27B_MMPROJ=ON(after supplying full lucebox-ggml / mtmd); text-only configure still builds with the flag OFF--mmprojreportsvision_supported: trueon/propssupports_multimodaldelegated)