feat(ds4v): dflash vision serving runtime with bounded IQ85 conversion - #722
Conversation
CPU-only tool turning abliterated Vision-Exp safetensors into ROCmFPX MIX GGUF. Down experts to qtype 105 with embedded P4MIXv1 codebooks. Gate and up experts to qtype 106 with a split GUMIXs1 sidecar. Vision, aligner, image and bias_vl tensors pass through losslessly. Calibration needs an imatrix file or explicit absmax-only. Includes block codec roundtrip plus layout plus rejection unit test.
This reverts commit 8e28cfb.
Qwen35Backend implements the image contract, so the engine now takes images on a single GPU with a dense Qwen model as well as with DS4V. - qwen35_vision: the qwen3vl_merger tower in plain ggml, read straight from the projector file published next to the model. Preprocessing follows the model's own rule (bicubic, sides to a multiple of 32, 64 to 1,024 tokens). - qwen35_image_prompt: one pad per image token, and the two-dimensional rotary positions image tokens take. Pure functions, covered by test_qwen35_image. - Prefill writes the encoded rows over the pad embeddings in its normal chunk loop; decode carries the position offset an image leaves behind. Image requests decode one token at a time, text keeps speculative decoding. - The target graph now uses interleaved M-RoPE, as the model is defined. For text all three axes hold the same position, so output is unchanged: byte identical to main on five prompts up to 19.6K tokens, same speed. - --mmproj is accepted for qwen35 on any backend; layer or tensor splits, remote shards and --max-concurrency still refuse it. On an R9700 with Qwen3.8-27B UD-IQ4_XS: AI2D 86/100, ChartQA 55/60 augmented and 42/60 human, image prefill 0.74 s on average, decode 31 to 35 tok/s. docs/ds4v-image-serving.md becomes docs/image-input.md and covers both models. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Main renamed the engine (luce_server, luce_common, luce:: namespaces, LUCE_* variables, luce.h). Every file this branch touches was merged three ways with the same rename applied to our side, so main's hand edits win and our additions use the new names. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- gpu_page_pool: an amdgpu card whose GTT counter exists but cannot be read now yields no estimate, instead of a total that leaves its pages out. - ds4_mix_converter: check the safetensors data offset for overflow before the bounds test, refuse FP8 values that decode to infinity, stop requiring tokenizer_config.json (never used), drop --layer-start (only 0 was ever accepted). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…shot an image prefill load_vision() runs again on unpark; the config and capability flag that request threads read are now written only the first time. An image prefill also drops any snapshot request: snapshots are keyed by tokens and pad tokens do not identify an image. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 existing issues remain and 12 new issues found across 110 files
Not reviewed (too large): server/deps/lodepng/lodepng.cpp (~7,244 lines), server/deps/lodepng/lodepng.h (~2,188 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/server/image_input.h">
<violation number="1" location="server/src/server/image_input.h:20">
P2: Qwen35 supports up to eight images, but the default transport limit rejects the fifth through eighth image before the backend can process it. Use a backend-specific image-count limit, or raise the common default to the backend-supported maximum.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_image_admission.h">
<violation number="1" location="server/src/deepseek4/deepseek4_image_admission.h:105">
P1: In a memory-limited cgroup, this admission can report success using host-wide `MemAvailable` even when the process cannot allocate the required host/UMA bytes. Make the live check cgroup-aware, or require callers to pass an effective process-available limit and use the minimum of it and `MemAvailable` before admitting.</violation>
</file>
<file name="server/CMakeLists.txt">
<violation number="1" location="server/CMakeLists.txt:1928">
P1: `test_ds4v_image_integration` has no implementation source for the assembly/materialization functions it calls, so the default test build fails at link time with undefined references. Add `deepseek4_image_assembly.cpp` to this target or link it against a library that provides those symbols.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_graph.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_graph.cpp:10137">
P2: When an image request follows a cached layer-major prefill, this helper leaves `ds4_layer_major_graph_caches` allocated even though its purpose is to retire all disposable decoder graphs before image admission. Clear those static layer-major graph caches here as well, otherwise valid image requests can fail memory admission because stale prefill arenas still consume GPU memory.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_image_spans.h">
<violation number="1" location="server/src/deepseek4/deepseek4_image_spans.h:8">
P3: The four-image cap introduced here as `DS4V_MAX_IMAGES = 4` is enforced as a literal `4` in sibling DS4V files, so the named limit can drift from actual enforcement. `deepseek4_image_prompt.cpp` rejects with `images.size()>4`, and `deepseek4_image_assembly.cpp` enforces `images.size() <= 4` in both `materialize_image_rows` and `embed_image_prompt_chunk`. Reference the header constant in those checks (or at least the ones that already include `deepseek4_image_spans.h`) so a limit change updates every enforcement point.</violation>
</file>
<file name="server/src/common/vision/image_decode.cpp">
<violation number="1" location="server/src/common/vision/image_decode.cpp:179">
P2: On 64-bit Windows, inputs larger than `ULONG_MAX` are truncated before libjpeg sees them, so a valid large image can be decoded from an incomplete buffer or rejected unpredictably. Reject lengths above `std::numeric_limits<unsigned long>::max()` before calling `jpeg_mem_src`.</violation>
<violation number="2" location="server/src/common/vision/image_decode.cpp:226">
P2: Any libjpeg warning (even one that does not impair pixel data) now fails the whole decode: `jpeg_emit_message` counts every negative-level message and the success path requires `error.warnings == 0`. Warnings such as "Corrupt JPEG data: N extraneous bytes before marker" (JWRN_EXTRANEOUS_DATA, emitted with level -1) occur routinely in valid real-world files (edited/progressive JPEGs, trailing bytes after EOI), and libjpeg's default handler treats them as recoverable. With this check those images are rejected as MalformedImage and users cannot attach them. If strictness is intended, gate it explicitly and degrade only on hard failure (`jpeg_finish_decompress` == FALSE); otherwise valid images that previously opened everywhere else will be refused.</violation>
</file>
<file name="server/test/test_image_resize.cpp">
<violation number="1" location="server/test/test_image_resize.cpp:1">
P3: The comment says the expected data was generated with Pillow 12.2.0, but common/vision/image_resize.h and image_resize.cpp both state the parity reference is Pillow 12.3.0 (Resample.c). If resampling changed between 12.2 and 12.3, regenerating these vectors against the stated reference version would fail; at minimum the versions must agree.</violation>
</file>
<file name="server/src/common/vision/image_resize.h">
<violation number="1" location="server/src/common/vision/image_resize.h:1">
P2: The header promises byte-for-byte parity with Pillow's Image.resize(BICUBIC), but the implementation it describes has a vertical_first branch that breaks that contract. In image_resize.cpp resize_impl(), when input_height > input_width * 100 && output_height < input_height, the code resizes vertically then horizontally, while Pillow's imageresample in src/libImaging/Resample.c always resizes horizontally then vertically. Because the intermediate buffer holds clipped 8-bit values (clip_fixed per pass), the two pass orderings are not bit-commutative, so that branch can emit pixels that differ from Pillow. The branch is also untested (all fixtures use normal aspect ratios). Either remove the vertical_first branch so every input takes Pillow's horizontal-then-vertical order, or keep the optimization but document the deviation from the byte-for-byte contract and add a fixture that pins the branch's output against Pillow.</violation>
</file>
<file name="server/test/test_qwen35_image.cpp">
<violation number="1" location="server/test/test_qwen35_image.cpp:114">
P3: This new pure-CPU test is registered as a standalone executable (`add_executable(test_qwen35_image ...)` in server/CMakeLists.txt), while the repository has an established unit-test framework: `server/test/CppUnitTestFramework.hpp`, and CMake already discovers framework-registered tests for `test_server_unit`, `test_model_smoke`, and other targets (server/CMakeLists.txt ~lines 2129, 2194, 2254). All four functions under test (`qwen35_vision_target_size`, `qwen35_vision_rope_positions`, `qwen35_vision_position_rows`, `qwen35_expand_image_tokens`) are pure and CPU-only, so they fit the framework's scope; adding a separate `main()`-based executable keeps this coverage outside the unified suite and its reporting. Convert the `check()`/`main()` harness to the framework's registration macros so the test runs and reports with the rest of the unit tests.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_image_admission.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_image_admission.cpp:274">
P3: The runtime admission path reports `storage_estimated = true` even though no storage estimate was performed. `check_deepseek4_image_runtime_admission` calls `assess_deepseek4_image_admission({}, activation_bytes, ...)` with an empty `ImageStorageEstimate`, but `assess` unconditionally marks `out.storage_estimated = true` and copies the empty struct into `out.storage`. Any consumer reading the flag believes expert payload/copy figures were derived from model metadata when they were actually left at zero. Set the flag only when the storage was computed (e.g., take a `storage_estimated` parameter, or only set it when the storage struct is non-trivial).</violation>
</file>
<file name="server/tools/ds4_mix_converter/ds4_mix_converter.cpp">
<violation number="1" location="server/tools/ds4_mix_converter/ds4_mix_converter.cpp:962">
P2: When `config.json` contains an all-zero or invalid-length-matching `compress_ratios` array, this code preserves it and bypasses the loader's validated default schedule, so the resulting model can silently use the wrong attention behavior. Validate the schedule before writing it, at least rejecting zero ratios beyond the first two layers and invalid ratio values.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 8 unresolved issues already reported by Cubic.
Re-trigger cubic
| // separates the metadata-derived activation estimate from reserved headroom. | ||
| // A successful snapshot is admission, not an allocation reservation or a proof | ||
| // against concurrent external allocations; guarded runtime verification remains | ||
| // necessary. Host cgroup/process limits must be included by caller policy. |
There was a problem hiding this comment.
P1: In a memory-limited cgroup, this admission can report success using host-wide MemAvailable even when the process cannot allocate the required host/UMA bytes. Make the live check cgroup-aware, or require callers to pass an effective process-available limit and use the minimum of it and MemAvailable before admitting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_image_admission.h, line 105:
<comment>In a memory-limited cgroup, this admission can report success using host-wide `MemAvailable` even when the process cannot allocate the required host/UMA bytes. Make the live check cgroup-aware, or require callers to pass an effective process-available limit and use the minimum of it and `MemAvailable` before admitting.</comment>
<file context>
@@ -0,0 +1,131 @@
+// separates the metadata-derived activation estimate from reserved headroom.
+// A successful snapshot is admission, not an allocation reservation or a proof
+// against concurrent external allocations; guarded runtime verification remains
+// necessary. Host cgroup/process limits must be included by caller policy.
+bool check_deepseek4_image_admission(
+ const common::DeepSeek4Weights & weights,
</file context>
| test_qwen35_image) | ||
|
|
||
| # DS4V image units: each test builds only the unit it covers. | ||
| foreach(_ds4v_unit assembly integration policy prompt) |
There was a problem hiding this comment.
P1: test_ds4v_image_integration has no implementation source for the assembly/materialization functions it calls, so the default test build fails at link time with undefined references. Add deepseek4_image_assembly.cpp to this target or link it against a library that provides those symbols.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/CMakeLists.txt, line 1928:
<comment>`test_ds4v_image_integration` has no implementation source for the assembly/materialization functions it calls, so the default test build fails at link time with undefined references. Add `deepseek4_image_assembly.cpp` to this target or link it against a library that provides those symbols.</comment>
<file context>
@@ -1840,6 +1902,46 @@ if(LUCE_TESTS)
+ test_qwen35_image)
+
+ # DS4V image units: each test builds only the unit it covers.
+ foreach(_ds4v_unit assembly integration policy prompt)
+ add_executable(test_ds4v_image_${_ds4v_unit} test/test_ds4v_image_${_ds4v_unit}.cpp)
+ target_include_directories(test_ds4v_image_${_ds4v_unit} PRIVATE
</file context>
| struct ImageInputLimits { | ||
| size_t image_bytes = MAX_IMAGE_BYTES; | ||
| size_t request_bytes = 32 * 1024 * 1024; | ||
| size_t image_count = 4; |
There was a problem hiding this comment.
P2: Qwen35 supports up to eight images, but the default transport limit rejects the fifth through eighth image before the backend can process it. Use a backend-specific image-count limit, or raise the common default to the backend-supported maximum.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/image_input.h, line 20:
<comment>Qwen35 supports up to eight images, but the default transport limit rejects the fifth through eighth image before the backend can process it. Use a backend-specific image-count limit, or raise the common default to the backend-supported maximum.</comment>
<file context>
@@ -0,0 +1,47 @@
+struct ImageInputLimits {
+ size_t image_bytes = MAX_IMAGE_BYTES;
+ size_t request_bytes = 32 * 1024 * 1024;
+ size_t image_count = 4;
+};
+
</file context>
|
|
||
| void deepseek4_release_image_scratch(DeepSeek4Cache & c, | ||
| MoeHybridStorage * moe_hybrid) { | ||
| deepseek4_release_prefill_scratch(c, moe_hybrid); |
There was a problem hiding this comment.
P2: When an image request follows a cached layer-major prefill, this helper leaves ds4_layer_major_graph_caches allocated even though its purpose is to retire all disposable decoder graphs before image admission. Clear those static layer-major graph caches here as well, otherwise valid image requests can fail memory admission because stale prefill arenas still consume GPU memory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_graph.cpp, line 10137:
<comment>When an image request follows a cached layer-major prefill, this helper leaves `ds4_layer_major_graph_caches` allocated even though its purpose is to retire all disposable decoder graphs before image admission. Clear those static layer-major graph caches here as well, otherwise valid image requests can fail memory admission because stale prefill arenas still consume GPU memory.</comment>
<file context>
@@ -9944,6 +10132,14 @@ void deepseek4_release_prefill_scratch(
+void deepseek4_release_image_scratch(DeepSeek4Cache & c,
+ MoeHybridStorage * moe_hybrid) {
+ deepseek4_release_prefill_scratch(c, moe_hybrid);
+ delete c.layer_range_cache;
+ c.layer_range_cache = nullptr;
</file context>
| @@ -0,0 +1,276 @@ | |||
| // resize_rgb_bicubic must reproduce Pillow's Image.resize(BICUBIC) byte for byte. | |||
There was a problem hiding this comment.
P3: The comment says the expected data was generated with Pillow 12.2.0, but common/vision/image_resize.h and image_resize.cpp both state the parity reference is Pillow 12.3.0 (Resample.c). If resampling changed between 12.2 and 12.3, regenerating these vectors against the stated reference version would fail; at minimum the versions must agree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/test/test_image_resize.cpp, line 1:
<comment>The comment says the expected data was generated with Pillow 12.2.0, but common/vision/image_resize.h and image_resize.cpp both state the parity reference is Pillow 12.3.0 (Resample.c). If resampling changed between 12.2 and 12.3, regenerating these vectors against the stated reference version would fail; at minimum the versions must agree.</comment>
<file context>
@@ -0,0 +1,276 @@
+// resize_rgb_bicubic must reproduce Pillow's Image.resize(BICUBIC) byte for byte.
+// Expected data generated with Pillow 12.2.0.
+#include "common/vision/image_resize.h"
</file context>
| check(chunk == std::vector<float>({30, 31, 40, 41, 0, 0}), "image tail continues in the next chunk"); | ||
| } | ||
|
|
||
| int main() { |
There was a problem hiding this comment.
P3: This new pure-CPU test is registered as a standalone executable (add_executable(test_qwen35_image ...) in server/CMakeLists.txt), while the repository has an established unit-test framework: server/test/CppUnitTestFramework.hpp, and CMake already discovers framework-registered tests for test_server_unit, test_model_smoke, and other targets (server/CMakeLists.txt ~lines 2129, 2194, 2254). All four functions under test (qwen35_vision_target_size, qwen35_vision_rope_positions, qwen35_vision_position_rows, qwen35_expand_image_tokens) are pure and CPU-only, so they fit the framework's scope; adding a separate main()-based executable keeps this coverage outside the unified suite and its reporting. Convert the check()/main() harness to the framework's registration macros so the test runs and reports with the rest of the unit tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/test/test_qwen35_image.cpp, line 114:
<comment>This new pure-CPU test is registered as a standalone executable (`add_executable(test_qwen35_image ...)` in server/CMakeLists.txt), while the repository has an established unit-test framework: `server/test/CppUnitTestFramework.hpp`, and CMake already discovers framework-registered tests for `test_server_unit`, `test_model_smoke`, and other targets (server/CMakeLists.txt ~lines 2129, 2194, 2254). All four functions under test (`qwen35_vision_target_size`, `qwen35_vision_rope_positions`, `qwen35_vision_position_rows`, `qwen35_expand_image_tokens`) are pure and CPU-only, so they fit the framework's scope; adding a separate `main()`-based executable keeps this coverage outside the unified suite and its reporting. Convert the `check()`/`main()` harness to the framework's registration macros so the test runs and reports with the rest of the unit tests.</comment>
<file context>
@@ -0,0 +1,122 @@
+ check(chunk == std::vector<float>({30, 31, 40, 41, 0, 0}), "image tail continues in the next chunk");
+}
+
+int main() {
+ test_target_size();
+ test_tower_order();
</file context>
| out = {}; | ||
| if (!estimate_deepseek4_image_storage(w, placement, config, primary, cold, | ||
| reserves.duplicate_hot_on_cold, out.storage, error)) return false; | ||
| out.storage_estimated = true; |
There was a problem hiding this comment.
P3: The runtime admission path reports storage_estimated = true even though no storage estimate was performed. check_deepseek4_image_runtime_admission calls assess_deepseek4_image_admission({}, activation_bytes, ...) with an empty ImageStorageEstimate, but assess unconditionally marks out.storage_estimated = true and copies the empty struct into out.storage. Any consumer reading the flag believes expert payload/copy figures were derived from model metadata when they were actually left at zero. Set the flag only when the storage was computed (e.g., take a storage_estimated parameter, or only set it when the storage struct is non-trivial).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_image_admission.cpp, line 274:
<comment>The runtime admission path reports `storage_estimated = true` even though no storage estimate was performed. `check_deepseek4_image_runtime_admission` calls `assess_deepseek4_image_admission({}, activation_bytes, ...)` with an empty `ImageStorageEstimate`, but `assess` unconditionally marks `out.storage_estimated = true` and copies the empty struct into `out.storage`. Any consumer reading the flag believes expert payload/copy figures were derived from model metadata when they were actually left at zero. Set the flag only when the storage was computed (e.g., take a `storage_estimated` parameter, or only set it when the storage struct is non-trivial).</comment>
<file context>
@@ -0,0 +1,396 @@
+ out = {};
+ if (!estimate_deepseek4_image_storage(w, placement, config, primary, cold,
+ reserves.duplicate_hot_on_cold, out.storage, error)) return false;
+ out.storage_estimated = true;
+ out.cold_runtime_reservation_bytes = reserves.cold_runtime_reservation_bytes;
+ if (!owner_activation_estimate(config, reserves.max_chunk_tokens,
</file context>
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Not reviewed (too large): server/deps/lodepng/lodepng.cpp (~7,244 lines), server/deps/lodepng/lodepng.h (~2,188 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/gpu_page_pool.cpp">
<violation number="1" location="server/src/common/gpu_page_pool.cpp:22">
P2: When `KReclaimable` contains kernel-misc pages outside `Slab`, this estimate adds them to `MemAvailable` a second time and can over-admit image storage. Account `KReclaimable` and the non-reclaimable slab component without double-counting slab pages.</violation>
</file>
<file name="server/test/test_ds4v_image_integration.cpp">
<violation number="1" location="server/test/test_ds4v_image_integration.cpp:216">
P2: This test file is not registered in server/CMakeLists.txt, so it is never compiled or run: 'make check' and ctest execute only the registered targets, and none of them includes this file. The new coverage therefore does nothing in CI. Register it, and follow the repo convention by integrating the checks into the CppUnit framework (test_unit_main.cpp + CppUnitTestFramework.hpp, registered via target_sources + luce_discover_cppunit_tests or add_test) instead of the custom main()/require() harness. The code is header-only (only src include path needed), so the wiring is small and can join _new_cppunit_test_targets in CMakeLists.txt.</violation>
</file>
<file name="server/src/server/http_server.cpp">
<violation number="1" location="server/src/server/http_server.cpp:2491">
P2: When automatic multi-model routing probes a vision model whose expanded prompt exceeds its context, this path returns 400 instead of trying another eligible model. Distinguish context-overflow preparation failures and mark them `RoutingAdmission::unfit` before returning.</violation>
</file>
<file name="server/tests/test_deepseek4_unit.cpp">
<violation number="1" location="server/tests/test_deepseek4_unit.cpp:263">
P3: The new `opts.block_count` parameterizes only `deepseek4.block_count` in the fixture; the other layer-count-dependent sections still hardcode 43. `std::vector<uint32_t> ratios(43, 4)` (compress-ratios, unchanged below the diff) writes a 43-element array for any model whose `block_count` differs, and the image-bias loop runs `layer < (opts.add_mtp_image_bias ? 44 : 43)`, emitting 43 biases regardless of `opts.block_count`. Any future test that sets `block_count != 43` together with `write_compress_ratios` or `image_biases` silently produces a malformed fixture (GGUF metadata disagrees with the tensor counts, so the loader rejects it for the wrong reason). Derive those counts from `opts.block_count` (e.g. `ratios(opts.block_count, 4)` and `layer < int(opts.block_count) + (opts.add_mtp_image_bias ? 1 : 0)`).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| // Fields that together describe every page /proc/meminfo can attribute. | ||
| constexpr const char * ACCOUNTED[] = { | ||
| "MemFree", "Buffers", "Cached", "SwapCached", "AnonPages", "Slab", |
There was a problem hiding this comment.
P2: When KReclaimable contains kernel-misc pages outside Slab, this estimate adds them to MemAvailable a second time and can over-admit image storage. Account KReclaimable and the non-reclaimable slab component without double-counting slab pages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/gpu_page_pool.cpp, line 22:
<comment>When `KReclaimable` contains kernel-misc pages outside `Slab`, this estimate adds them to `MemAvailable` a second time and can over-admit image storage. Account `KReclaimable` and the non-reclaimable slab component without double-counting slab pages.</comment>
<file context>
@@ -0,0 +1,89 @@
+
+// Fields that together describe every page /proc/meminfo can attribute.
+constexpr const char * ACCOUNTED[] = {
+ "MemFree", "Buffers", "Cached", "SwapCached", "AnonPages", "Slab",
+ "KernelStack", "PageTables", "SecPageTables", "Percpu", "Bounce",
+};
</file context>
| @@ -0,0 +1,228 @@ | |||
| #include "deepseek4_image_budget.h" | |||
There was a problem hiding this comment.
P2: This test file is not registered in server/CMakeLists.txt, so it is never compiled or run: 'make check' and ctest execute only the registered targets, and none of them includes this file. The new coverage therefore does nothing in CI. Register it, and follow the repo convention by integrating the checks into the CppUnit framework (test_unit_main.cpp + CppUnitTestFramework.hpp, registered via target_sources + luce_discover_cppunit_tests or add_test) instead of the custom main()/require() harness. The code is header-only (only src include path needed), so the wiring is small and can join _new_cppunit_test_targets in CMakeLists.txt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/test/test_ds4v_image_integration.cpp, line 216:
<comment>This test file is not registered in server/CMakeLists.txt, so it is never compiled or run: 'make check' and ctest execute only the registered targets, and none of them includes this file. The new coverage therefore does nothing in CI. Register it, and follow the repo convention by integrating the checks into the CppUnit framework (test_unit_main.cpp + CppUnitTestFramework.hpp, registered via target_sources + luce_discover_cppunit_tests or add_test) instead of the custom main()/require() harness. The code is header-only (only src include path needed), so the wiring is small and can join _new_cppunit_test_targets in CMakeLists.txt.</comment>
<file context>
@@ -0,0 +1,228 @@
+}
+} // namespace
+
+int main() {
+ try {
+ validation_and_lookup();
</file context>
| if (!render_and_tokenize_request(fd, render_messages, req)) return true; | ||
|
|
||
| std::string image_error; | ||
| if (!backend_.prepare_images(req.prompt_tokens, std::move(encoded_images), |
There was a problem hiding this comment.
P2: When automatic multi-model routing probes a vision model whose expanded prompt exceeds its context, this path returns 400 instead of trying another eligible model. Distinguish context-overflow preparation failures and mark them RoutingAdmission::unfit before returning.
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 2491:
<comment>When automatic multi-model routing probes a vision model whose expanded prompt exceeds its context, this path returns 400 instead of trying another eligible model. Distinguish context-overflow preparation failures and mark them `RoutingAdmission::unfit` before returning.</comment>
<file context>
@@ -2463,6 +2487,14 @@ bool HttpServer::handle_model_request(SocketHandle fd, ParsedRequest & req,
if (!render_and_tokenize_request(fd, render_messages, req)) return true;
+ std::string image_error;
+ if (!backend_.prepare_images(req.prompt_tokens, std::move(encoded_images),
+ uint64_t(std::max(0, config_.max_ctx)), uint64_t(std::max(0, req.max_output)),
+ req.images, image_error)) {
</file context>
| gguf_context * g = gguf_init_empty(); | ||
| gguf_set_val_str(g, "general.architecture", "deepseek4"); | ||
| gguf_set_val_u32(g, "deepseek4.block_count", 43); | ||
| gguf_set_val_u32(g, "deepseek4.block_count", opts.block_count); |
There was a problem hiding this comment.
P3: The new opts.block_count parameterizes only deepseek4.block_count in the fixture; the other layer-count-dependent sections still hardcode 43. std::vector<uint32_t> ratios(43, 4) (compress-ratios, unchanged below the diff) writes a 43-element array for any model whose block_count differs, and the image-bias loop runs layer < (opts.add_mtp_image_bias ? 44 : 43), emitting 43 biases regardless of opts.block_count. Any future test that sets block_count != 43 together with write_compress_ratios or image_biases silently produces a malformed fixture (GGUF metadata disagrees with the tensor counts, so the loader rejects it for the wrong reason). Derive those counts from opts.block_count (e.g. ratios(opts.block_count, 4) and layer < int(opts.block_count) + (opts.add_mtp_image_bias ? 1 : 0)).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/test_deepseek4_unit.cpp, line 263:
<comment>The new `opts.block_count` parameterizes only `deepseek4.block_count` in the fixture; the other layer-count-dependent sections still hardcode 43. `std::vector<uint32_t> ratios(43, 4)` (compress-ratios, unchanged below the diff) writes a 43-element array for any model whose `block_count` differs, and the image-bias loop runs `layer < (opts.add_mtp_image_bias ? 44 : 43)`, emitting 43 biases regardless of `opts.block_count`. Any future test that sets `block_count != 43` together with `write_compress_ratios` or `image_biases` silently produces a malformed fixture (GGUF metadata disagrees with the tensor counts, so the loader rejects it for the wrong reason). Derive those counts from `opts.block_count` (e.g. `ratios(opts.block_count, 4)` and `layer < int(opts.block_count) + (opts.add_mtp_image_bias ? 1 : 0)`).</comment>
<file context>
@@ -249,7 +260,7 @@ static std::string make_temp_gguf_path(const char * prefix) {
gguf_context * g = gguf_init_empty();
gguf_set_val_str(g, "general.architecture", "deepseek4");
- gguf_set_val_u32(g, "deepseek4.block_count", 43);
+ gguf_set_val_u32(g, "deepseek4.block_count", opts.block_count);
gguf_set_val_u32(g, "deepseek4.embedding_length", 4096);
if (opts.include_vocab_size) {
</file context>
…ader and DS4V admission - qwen35: a text request that restores an exact snapshot runs no prefill, so it could decode with the position offset the previous image request left behind. The offset is now reset on every restore. Checked with text, image, same text again: the restored request answers identically. - qwen35: a failed projector reload on unpark frees the target again instead of leaving it resident but marked parked; allocation failures while encoding become a request error and still release the tower scratch; the image count matches the server's transport limit of four. - deepseek4: with the whole model on one GPU, the per-layer prefill graph arenas are released before an image is admitted and rebuilt on demand. - server: warn at startup when a projector is loaded but image input ends up disabled (upstream forwarding or concurrent sequence scheduling). - mmproj_file: include <cstddef>, no pointer arithmetic on an empty array. - image_resize.h documents the one case that departs from Pillow's pass order. - docs: the image count and size limits are shared by every model; Qwen results on a Strix Halo alone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 110 files
Not reviewed (too large): server/deps/lodepng/lodepng.cpp (~7,244 lines), server/deps/lodepng/lodepng.h (~2,188 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/deepseek4/deepseek4_backend.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_backend.cpp:1072">
P2: When a second image request arrives while an earlier one is still generating, `try_acquire()` returns null and the HTTP layer answers 400 "an image request is already in progress". That is a server-saturation condition surfacing as a permanent client error. The server job queue already serializes generation, so a blocking acquire (or a queued/503 with retry semantics) would let the second request wait out the first instead of failing it.</violation>
</file>
<file name="server/src/qwen35/qwen35_backend_images.cpp">
<violation number="1" location="server/src/qwen35/qwen35_backend_images.cpp:33">
P3: `vision_config_` is captured once and never refreshed, while `vision_` itself is fully rebuilt on every unpark reload. If the projector file at `--mmproj` changes between park and unpark (or the first load observed different metadata), the live tower and the config used by `prepare_images`/`qwen35_vision_preprocess` (grid columns/rows, max tokens per image) can disagree, producing wrong preprocessing silently. Refresh `vision_config_` from `tower->config()` on every reload instead of guarding with `image_input_` (reloads only happen under park, when no requests are running, so the single-writer invariant still holds).</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| error = "too many images in request"; | ||
| return false; | ||
| } | ||
| auto lease = image_request_gate_.try_acquire(); |
There was a problem hiding this comment.
P2: When a second image request arrives while an earlier one is still generating, try_acquire() returns null and the HTTP layer answers 400 "an image request is already in progress". That is a server-saturation condition surfacing as a permanent client error. The server job queue already serializes generation, so a blocking acquire (or a queued/503 with retry semantics) would let the second request wait out the first instead of failing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_backend.cpp, line 1072:
<comment>When a second image request arrives while an earlier one is still generating, `try_acquire()` returns null and the HTTP layer answers 400 "an image request is already in progress". That is a server-saturation condition surfacing as a permanent client error. The server job queue already serializes generation, so a blocking acquire (or a queued/503 with retry semantics) would let the second request wait out the first instead of failing it.</comment>
<file context>
@@ -991,6 +1041,245 @@ DeepSeek4Backend::~DeepSeek4Backend() {
+ error = "too many images in request";
+ return false;
+ }
+ auto lease = image_request_gate_.try_acquire();
+ if (!lease) {
+ error = "an image request is already in progress; retry after it completes";
</file context>
| } | ||
| // Request threads read these two without a lock, so they are written | ||
| // once: a reload after unpark finds them already set to the same values. | ||
| if (!image_input_) { |
There was a problem hiding this comment.
P3: vision_config_ is captured once and never refreshed, while vision_ itself is fully rebuilt on every unpark reload. If the projector file at --mmproj changes between park and unpark (or the first load observed different metadata), the live tower and the config used by prepare_images/qwen35_vision_preprocess (grid columns/rows, max tokens per image) can disagree, producing wrong preprocessing silently. Refresh vision_config_ from tower->config() on every reload instead of guarding with image_input_ (reloads only happen under park, when no requests are running, so the single-writer invariant still holds).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_backend_images.cpp, line 33:
<comment>`vision_config_` is captured once and never refreshed, while `vision_` itself is fully rebuilt on every unpark reload. If the projector file at `--mmproj` changes between park and unpark (or the first load observed different metadata), the live tower and the config used by `prepare_images`/`qwen35_vision_preprocess` (grid columns/rows, max tokens per image) can disagree, producing wrong preprocessing silently. Refresh `vision_config_` from `tower->config()` on every reload instead of guarding with `image_input_` (reloads only happen under park, when no requests are running, so the single-writer invariant still holds).</comment>
<file context>
@@ -0,0 +1,112 @@
+ }
+ // Request threads read these two without a lock, so they are written
+ // once: a reload after unpark finds them already set to the same values.
+ if (!image_input_) {
+ vision_config_ = tower->config();
+ image_input_ = true;
</file context>
… items - qwen35: shutdown resets the vision tower before freeing the target backend that owns its buffers; a projector reloaded after unpark must have the same geometry as the one request threads already preprocess for. - One spelling of the image pad token, shared by the loader and the prompt. - gpu_page_pool: use meminfo's Hugetlb total when present, which covers huge page pools of every size (test added). - MmprojFile documents that a failed load leaves the object unusable; drop the unused decode_error_name(); tests include <cstring> for what they use. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 110 files
Not reviewed (too large): server/deps/lodepng/lodepng.cpp (~7,244 lines), server/deps/lodepng/lodepng.h (~2,188 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/test/test_moe_source_page_range.cpp">
<violation number="1" location="server/test/test_moe_source_page_range.cpp:71">
P3: This is a hand-rolled standalone executable (own `main()`, `check()`/`std::exit`), but the repo already has an established host-side unit-test framework: `server/test/CppUnitTestFramework.hpp` with `server/test/test_unit_main.cpp` (`#define GENERATE_UNIT_TEST_MAIN`). Host-only logic tests such as `test_kernel_qualification_core.cpp` and `test_feature_gate.cpp` are written as `TEST_CASE` fixtures, and `server/CMakeLists.txt` keeps the `_new_cppunit_test_targets` list that wires framework targets into `luce_discover_cppunit_tests` for per-case ctest discovery and keyword filtering. This new executable is registered only as a coarse standalone target (CMakeLists lines 1941–1943), so every assertion failure reports as one opaque ctest failure instead of a named case. Convert the host checks into `TEST_CASE` fixtures and add the target to `_new_cppunit_test_targets` so the coverage integrates with the established suite rather than introducing a separate executable. (Based on your team's feedback about extending tests through the existing test framework.)</violation>
<violation number="2" location="server/test/test_moe_source_page_range.cpp:121">
P3: The `errno = EBUSY` pre-seed does not test what the comment claims. In `reclaim_copied_file_source` (server/src/common/copied_source_reclaim.h), `errno` is only read inside the `if (::madvise(...) != 0) result.madvise_error = errno;` branch, so on a successful `madvise` the pre-seeded value is never observed and `advice.madvise_error == 0` still passes even if `errno` stayed `EBUSY`. The line is inert; drop it and the misleading comment, or restructure the check to actually capture/assert `errno` around the advice call.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| reinterpret_cast<uintptr_t>(mapped) + 17, 3 * p + 100, p, range), "native source range"); | ||
| std::vector<unsigned char> before(5), after(5); | ||
| const int before_rc = ::mincore(mapped, size, before.data()); | ||
| errno = EBUSY; // Successful advice must not report a stale errno. |
There was a problem hiding this comment.
P3: The errno = EBUSY pre-seed does not test what the comment claims. In reclaim_copied_file_source (server/src/common/copied_source_reclaim.h), errno is only read inside the if (::madvise(...) != 0) result.madvise_error = errno; branch, so on a successful madvise the pre-seeded value is never observed and advice.madvise_error == 0 still passes even if errno stayed EBUSY. The line is inert; drop it and the misleading comment, or restructure the check to actually capture/assert errno around the advice call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/test/test_moe_source_page_range.cpp, line 121:
<comment>The `errno = EBUSY` pre-seed does not test what the comment claims. In `reclaim_copied_file_source` (server/src/common/copied_source_reclaim.h), `errno` is only read inside the `if (::madvise(...) != 0) result.madvise_error = errno;` branch, so on a successful `madvise` the pre-seeded value is never observed and `advice.madvise_error == 0` still passes even if `errno` stayed `EBUSY`. The line is inert; drop it and the misleading comment, or restructure the check to actually capture/assert `errno` around the advice call.</comment>
<file context>
@@ -0,0 +1,148 @@
+ reinterpret_cast<uintptr_t>(mapped) + 17, 3 * p + 100, p, range), "native source range");
+ std::vector<unsigned char> before(5), after(5);
+ const int before_rc = ::mincore(mapped, size, before.data());
+ errno = EBUSY; // Successful advice must not report a stale errno.
+ const auto advice = reclaim_copied_file_source(mapped, size,
+ static_cast<const uint8_t *>(mapped) + 17, 3 * p + 100, fd, "test");
</file context>
| check(small_calls == 1, "short tensor uploaded once"); | ||
| } | ||
|
|
||
| int main() { |
There was a problem hiding this comment.
P3: This is a hand-rolled standalone executable (own main(), check()/std::exit), but the repo already has an established host-side unit-test framework: server/test/CppUnitTestFramework.hpp with server/test/test_unit_main.cpp (#define GENERATE_UNIT_TEST_MAIN). Host-only logic tests such as test_kernel_qualification_core.cpp and test_feature_gate.cpp are written as TEST_CASE fixtures, and server/CMakeLists.txt keeps the _new_cppunit_test_targets list that wires framework targets into luce_discover_cppunit_tests for per-case ctest discovery and keyword filtering. This new executable is registered only as a coarse standalone target (CMakeLists lines 1941–1943), so every assertion failure reports as one opaque ctest failure instead of a named case. Convert the host checks into TEST_CASE fixtures and add the target to _new_cppunit_test_targets so the coverage integrates with the established suite rather than introducing a separate executable.
(Based on your team's feedback about extending tests through the existing test framework.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/test/test_moe_source_page_range.cpp, line 71:
<comment>This is a hand-rolled standalone executable (own `main()`, `check()`/`std::exit`), but the repo already has an established host-side unit-test framework: `server/test/CppUnitTestFramework.hpp` with `server/test/test_unit_main.cpp` (`#define GENERATE_UNIT_TEST_MAIN`). Host-only logic tests such as `test_kernel_qualification_core.cpp` and `test_feature_gate.cpp` are written as `TEST_CASE` fixtures, and `server/CMakeLists.txt` keeps the `_new_cppunit_test_targets` list that wires framework targets into `luce_discover_cppunit_tests` for per-case ctest discovery and keyword filtering. This new executable is registered only as a coarse standalone target (CMakeLists lines 1941–1943), so every assertion failure reports as one opaque ctest failure instead of a named case. Convert the host checks into `TEST_CASE` fixtures and add the target to `_new_cppunit_test_targets` so the coverage integrates with the established suite rather than introducing a separate executable.
(Based on your team's feedback about extending tests through the existing test framework.) </comment>
<file context>
@@ -0,0 +1,148 @@
+ check(small_calls == 1, "short tensor uploaded once");
+}
+
+int main() {
+ check_staged_upload();
+ constexpr size_t page = 4096;
</file context>
The tower attended the naive way: an F32 score matrix per head (3,900 patches squared for a 1,024-token image) through two GEMMs and a softmax, 41% of the encoder's time on an R9700. It now uses ggml's fused attention with half-precision keys and values, as the reference implementation does. Measured on the R9700 with the BF16 projector: a 975-token image encodes in 677 ms instead of 794 ms, which matches llama.cpp's 669 ms on the same image. On the 220-question AI2D/ChartQA set the scores are 85/100, 55/60 and 43/60 (before: 86, 55, 42), with 211 of 220 answers identical. The backend also logs the encode time per image request. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 110 files
Not reviewed (too large): server/deps/lodepng/lodepng.cpp (~7,244 lines), server/deps/lodepng/lodepng.h (~2,188 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/qwen35/qwen35_backend_images.cpp">
<violation number="1" location="server/src/qwen35/qwen35_backend_images.cpp:70">
P3: Requests whose expanded prompt exceeds `context_capacity - output_reserve` are rejected only after every image has been decoded, resized, and normalized: the `limit` check runs inside `qwen35_expand_image_tokens`, after the decode/preprocess loop. A multi-image request with a long text prompt therefore pays the full decode + bicubic-resize cost before rejection. Compute the projected token total from `qwen35_vision_target_size` right after decoding and bail before resizing/normalizing.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| try { | ||
| auto prompt = std::make_shared<Qwen35ImagePrompt>(); | ||
| prompt->owner = this; | ||
| for (const EncodedImage & image : images) { |
There was a problem hiding this comment.
P3: Requests whose expanded prompt exceeds context_capacity - output_reserve are rejected only after every image has been decoded, resized, and normalized: the limit check runs inside qwen35_expand_image_tokens, after the decode/preprocess loop. A multi-image request with a long text prompt therefore pays the full decode + bicubic-resize cost before rejection. Compute the projected token total from qwen35_vision_target_size right after decoding and bail before resizing/normalizing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_backend_images.cpp, line 70:
<comment>Requests whose expanded prompt exceeds `context_capacity - output_reserve` are rejected only after every image has been decoded, resized, and normalized: the `limit` check runs inside `qwen35_expand_image_tokens`, after the decode/preprocess loop. A multi-image request with a long text prompt therefore pays the full decode + bicubic-resize cost before rejection. Compute the projected token total from `qwen35_vision_target_size` right after decoding and bail before resizing/normalizing.</comment>
<file context>
@@ -0,0 +1,125 @@
+ try {
+ auto prompt = std::make_shared<Qwen35ImagePrompt>();
+ prompt->owner = this;
+ for (const EncodedImage & image : images) {
+ auto decoded = vision::decode_image({image.bytes.data(), image.bytes.size()});
+ if (!decoded) { error = decoded.status.message; return false; }
</file context>
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Not reviewed (too large): server/deps/lodepng/lodepng.cpp (~7,244 lines), server/deps/lodepng/lodepng.h (~2,188 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/qwen35/qwen35_vision.cpp">
<violation number="1" location="server/src/qwen35/qwen35_vision.cpp:64">
P3: The grow branch rounds up both dimensions to the next factor multiple but never re-checks the upper bound, so w*h can end up above max_pixels even though it entered the branch with area below min_pixels. The shrink branch enforces its bound (floor) but the grow branch only enforces the lower one, so the documented contract "area within the configured token bounds" is not guaranteed; a projector with min/max tokens close together (the config values are caller-tunable, not read from the file) can get an image that later trips the encode() guard `tokens > c.max_image_tokens`. Re-apply the shrink check to the grown size, as the reference clamping does.</violation>
<violation number="2" location="server/src/qwen35/qwen35_vision.cpp:160">
P2: The layer cap of 256 is inconsistent with the encode() graph budget: each block adds ~29 nodes (norm/mul/add per LN, qkv linear, 3 views, 2 ropes, 2 casts, 3 permutes, flash attn, reshape, out linear, residual add, MLP linear + gelu), plus ~22 prologue/epilogue nodes. GRAPH_NODES is 2048, so a projector with more than ~70 blocks that passes load() will overflow the graph in ggml_build_forward_expand, which trips ggml's node-capacity assertion (process abort) while serving. Cap layers from the node budget with a safety margin so an accepted-but-unsupported projector fails cleanly at load() with the existing error message instead of crashing at encode time.</violation>
<violation number="3" location="server/src/qwen35/qwen35_vision.cpp:311">
P1: Images with more than one patch are assembled in a different order from the rotary positions and 2x2 merger contract. Permute the convolution output to put the channel dimension first before the existing reshape/interleave sequence.</violation>
</file>
<file name="docs/image-input.md">
<violation number="1" location="docs/image-input.md:9">
P3: The Qwen row claims "one GPU, any backend", but the Qwen verification section below explicitly marks CUDA as "Not yet established". State in the table that only HIP has been verified (matching the DS4V row), or drop the "any backend" claim until CUDA is measured.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ggml_conv_2d(ctx, m.patch_second, image, p, p, 0, 0, 1, 1)); | ||
| // [columns, lines, d] in raster order -> [d, patches] with the four | ||
| // patches of each 2x2 block adjacent, which is what the merger expects. | ||
| x = ggml_permute(ctx, x, 1, 2, 0, 3); |
There was a problem hiding this comment.
P1: Images with more than one patch are assembled in a different order from the rotary positions and 2x2 merger contract. Permute the convolution output to put the channel dimension first before the existing reshape/interleave sequence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_vision.cpp, line 311:
<comment>Images with more than one patch are assembled in a different order from the rotary positions and 2x2 merger contract. Permute the convolution output to put the channel dimension first before the existing reshape/interleave sequence.</comment>
<file context>
@@ -0,0 +1,382 @@
+ ggml_conv_2d(ctx, m.patch_second, image, p, p, 0, 0, 1, 1));
+ // [columns, lines, d] in raster order -> [d, patches] with the four
+ // patches of each 2x2 block adjacent, which is what the merger expects.
+ x = ggml_permute(ctx, x, 1, 2, 0, 3);
+ x = ggml_cont_4d(ctx, x, d * 2, columns / 2, lines, 1);
+ x = ggml_reshape_4d(ctx, x, d * 2, columns / 2, 2, lines / 2);
</file context>
| x = ggml_permute(ctx, x, 1, 2, 0, 3); | |
| x = ggml_permute(ctx, x, 2, 0, 1, 3); |
| } | ||
| // The patch reordering in encode() is written for 2x2 merging, and | ||
| // the vision rope splits each head into four equal sections. | ||
| if (layers == 0 || layers > 256 || heads == 0 || dimension == 0 || dimension > 16384 || |
There was a problem hiding this comment.
P2: The layer cap of 256 is inconsistent with the encode() graph budget: each block adds ~29 nodes (norm/mul/add per LN, qkv linear, 3 views, 2 ropes, 2 casts, 3 permutes, flash attn, reshape, out linear, residual add, MLP linear + gelu), plus ~22 prologue/epilogue nodes. GRAPH_NODES is 2048, so a projector with more than ~70 blocks that passes load() will overflow the graph in ggml_build_forward_expand, which trips ggml's node-capacity assertion (process abort) while serving. Cap layers from the node budget with a safety margin so an accepted-but-unsupported projector fails cleanly at load() with the existing error message instead of crashing at encode time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_vision.cpp, line 160:
<comment>The layer cap of 256 is inconsistent with the encode() graph budget: each block adds ~29 nodes (norm/mul/add per LN, qkv linear, 3 views, 2 ropes, 2 casts, 3 permutes, flash attn, reshape, out linear, residual add, MLP linear + gelu), plus ~22 prologue/epilogue nodes. GRAPH_NODES is 2048, so a projector with more than ~70 blocks that passes load() will overflow the graph in ggml_build_forward_expand, which trips ggml's node-capacity assertion (process abort) while serving. Cap layers from the node budget with a safety margin so an accepted-but-unsupported projector fails cleanly at load() with the existing error message instead of crashing at encode time.</comment>
<file context>
@@ -0,0 +1,382 @@
+ }
+ // The patch reordering in encode() is written for 2x2 merging, and
+ // the vision rope splits each head into four equal sections.
+ if (layers == 0 || layers > 256 || heads == 0 || dimension == 0 || dimension > 16384 ||
+ dimension % heads != 0 || (dimension / heads) % 4 != 0 || intermediate == 0 ||
+ patch == 0 || patch > 64 || merge != 2) {
</file context>
| if (layers == 0 || layers > 256 || heads == 0 || dimension == 0 || dimension > 16384 || | |
| if (layers == 0 || layers > 64 || heads == 0 || dimension == 0 || dimension > 16384 || |
|
|
||
| | Model | Projector file | Runs on | | ||
| | --- | --- | --- | | ||
| | Qwen3.5 / Qwen3.8 dense | the `mmproj-*.gguf` published next to the model (llama.cpp `clip` format, type `qwen3vl_merger`) | one GPU, any backend | |
There was a problem hiding this comment.
P3: The Qwen row claims "one GPU, any backend", but the Qwen verification section below explicitly marks CUDA as "Not yet established". State in the table that only HIP has been verified (matching the DS4V row), or drop the "any backend" claim until CUDA is measured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/image-input.md, line 9:
<comment>The Qwen row claims "one GPU, any backend", but the Qwen verification section below explicitly marks CUDA as "Not yet established". State in the table that only HIP has been verified (matching the DS4V row), or drop the "any backend" claim until CUDA is measured.</comment>
<file context>
@@ -0,0 +1,199 @@
+
+| Model | Projector file | Runs on |
+| --- | --- | --- |
+| Qwen3.5 / Qwen3.8 dense | the `mmproj-*.gguf` published next to the model (llama.cpp `clip` format, type `qwen3vl_merger`) | one GPU, any backend |
+| DeepSeek V4 Flash Vision (DS4V) | [exported with our tool](ds4v-mmproj.md) | HIP: one GPU, or two GPUs splitting the experts |
+
</file context>
| | Qwen3.5 / Qwen3.8 dense | the `mmproj-*.gguf` published next to the model (llama.cpp `clip` format, type `qwen3vl_merger`) | one GPU, any backend | | |
| | Qwen3.5 / Qwen3.8 dense | the `mmproj-*.gguf` published next to the model (llama.cpp `clip` format, type `qwen3vl_merger`) | HIP: one GPU (CUDA not yet established) | |
| w = int(std::ceil(width * grow / factor)) * factor; | ||
| h = int(std::ceil(height * grow / factor)) * factor; |
There was a problem hiding this comment.
P3: The grow branch rounds up both dimensions to the next factor multiple but never re-checks the upper bound, so w*h can end up above max_pixels even though it entered the branch with area below min_pixels. The shrink branch enforces its bound (floor) but the grow branch only enforces the lower one, so the documented contract "area within the configured token bounds" is not guaranteed; a projector with min/max tokens close together (the config values are caller-tunable, not read from the file) can get an image that later trips the encode() guard tokens > c.max_image_tokens. Re-apply the shrink check to the grown size, as the reference clamping does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_vision.cpp, line 64:
<comment>The grow branch rounds up both dimensions to the next factor multiple but never re-checks the upper bound, so w*h can end up above max_pixels even though it entered the branch with area below min_pixels. The shrink branch enforces its bound (floor) but the grow branch only enforces the lower one, so the documented contract "area within the configured token bounds" is not guaranteed; a projector with min/max tokens close together (the config values are caller-tunable, not read from the file) can get an image that later trips the encode() guard `tokens > c.max_image_tokens`. Re-apply the shrink check to the grown size, as the reference clamping does.</comment>
<file context>
@@ -0,0 +1,382 @@
+ h = std::max(factor, int(std::floor(height / shrink / factor)) * factor);
+ } else if (double(w) * h < min_pixels) {
+ const double grow = std::sqrt(min_pixels / area);
+ w = int(std::ceil(width * grow / factor)) * factor;
+ h = int(std::ceil(height * grow / factor)) * factor;
+ }
</file context>
| w = int(std::ceil(width * grow / factor)) * factor; | |
| h = int(std::ceil(height * grow / factor)) * factor; | |
| w = int(std::ceil(width * grow / factor)) * factor; | |
| h = int(std::ceil(height * grow / factor)) * factor; | |
| if (double(w) * h > max_pixels) { | |
| w = std::max(factor, int(std::floor(width / shrink_of(w, h) / factor)) * factor); | |
| } |
The codebook fit and the encode pass ran one expert at a time on one core: about 32 minutes for the fit and 10 hours for the encode of a 43-layer, 256-expert checkpoint. Both passes now run their per-expert work on a pool of threads and consume the results in expert order, so the output is byte for byte the file the sequential converter wrote (checked with SHA-256 on a one-layer slice) and the unit test is unchanged. --threads N overrides the core count. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The layer-major prefill built the attention output projection in the grouped source layout unconditionally. That layout is read by MMQ's activation quantizer and by nothing else, so a model whose attn_output_b is stored unquantized (BF16, as the MIX converter writes dense tensors) aborted on its first prefill with GGML_ASSERT(use_mul_mat_q). Such a tensor now takes the plain projection path; quantized files are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
general.name was a hardcoded string from the contributor's test checkpoint; it now comes from the config's _name_or_path or the input directory. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 110 files
Not reviewed (too large): server/deps/lodepng/lodepng.cpp (~7,244 lines), server/deps/lodepng/lodepng.h (~2,188 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/tools/ds4_mix_converter/ds4_mix_converter.cpp">
<violation number="1" location="server/tools/ds4_mix_converter/ds4_mix_converter.cpp:110">
P2: If `consume(i, result)` throws (e.g. `fwrite_exact` fails on ENOSPC inside `write_expert_tensor`), the exception propagates out of `for_each_expert_ordered` while all pool threads are still joinable, so destroying `pool` calls `std::terminate()` instead of reaching the clean ERROR path in `main`. Wrap the consume step: on throw, record it, break out, join the pool, then rethrow the recorded exception.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_graph.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_graph.cpp:6735">
P2: Each image token goes through `select_image_experts`, which validates all n_expert scores/bias with `std::isfinite` and performs a full O(E log E) sort on the host, once per layer per chunk, inside the hybrid FFN loop — even for chunks where the bias was already validated wholesale in `deepseek4_validate_image_batch`. For the typical 448-token image × 40 layers that is ~18K full sorts of up to 256 entries in the prefill hot path, on top of the existing per-token routing. This is also the only place image routing weights are normalized as `(scores[order[i]]/sum)*route_scale` with `route_scale = w.expert_weight_scale`; the graph-side path (`build_moe_routing` top-k over `probs + selection_bias`) computes weights by a different route, so single-GPU vs two-GPU image output parity depends on those two normalizations agreeing. Worth reusing the validated GPU top-k result instead of re-deriving it on the host, and confirming the two weighings match exactly (the 213/220 identical-answer result suggests only borderline tokens diverge).</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| consumed = i + 1; | ||
| } | ||
| ready.notify_all(); | ||
| consume(i, result); |
There was a problem hiding this comment.
P2: If consume(i, result) throws (e.g. fwrite_exact fails on ENOSPC inside write_expert_tensor), the exception propagates out of for_each_expert_ordered while all pool threads are still joinable, so destroying pool calls std::terminate() instead of reaching the clean ERROR path in main. Wrap the consume step: on throw, record it, break out, join the pool, then rethrow the recorded exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tools/ds4_mix_converter/ds4_mix_converter.cpp, line 110:
<comment>If `consume(i, result)` throws (e.g. `fwrite_exact` fails on ENOSPC inside `write_expert_tensor`), the exception propagates out of `for_each_expert_ordered` while all pool threads are still joinable, so destroying `pool` calls `std::terminate()` instead of reaching the clean ERROR path in `main`. Wrap the consume step: on throw, record it, break out, join the pool, then rethrow the recorded exception.</comment>
<file context>
@@ -0,0 +1,1499 @@
+ consumed = i + 1;
+ }
+ ready.notify_all();
+ consume(i, result);
+ }
+ for (auto & t : pool) t.join();
</file context>
| float * token_weights = | ||
| weights.data() + (size_t)t * (size_t)route_width; | ||
|
|
||
| if (vision::image_block_at(image_spans, uint64_t(kv_start + t))) { |
There was a problem hiding this comment.
P2: Each image token goes through select_image_experts, which validates all n_expert scores/bias with std::isfinite and performs a full O(E log E) sort on the host, once per layer per chunk, inside the hybrid FFN loop — even for chunks where the bias was already validated wholesale in deepseek4_validate_image_batch. For the typical 448-token image × 40 layers that is ~18K full sorts of up to 256 entries in the prefill hot path, on top of the existing per-token routing. This is also the only place image routing weights are normalized as (scores[order[i]]/sum)*route_scale with route_scale = w.expert_weight_scale; the graph-side path (build_moe_routing top-k over probs + selection_bias) computes weights by a different route, so single-GPU vs two-GPU image output parity depends on those two normalizations agreeing. Worth reusing the validated GPU top-k result instead of re-deriving it on the host, and confirming the two weighings match exactly (the 213/220 identical-answer result suggests only borderline tokens diverge).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_graph.cpp, line 6735:
<comment>Each image token goes through `select_image_experts`, which validates all n_expert scores/bias with `std::isfinite` and performs a full O(E log E) sort on the host, once per layer per chunk, inside the hybrid FFN loop — even for chunks where the bias was already validated wholesale in `deepseek4_validate_image_batch`. For the typical 448-token image × 40 layers that is ~18K full sorts of up to 256 entries in the prefill hot path, on top of the existing per-token routing. This is also the only place image routing weights are normalized as `(scores[order[i]]/sum)*route_scale` with `route_scale = w.expert_weight_scale`; the graph-side path (`build_moe_routing` top-k over `probs + selection_bias`) computes weights by a different route, so single-GPU vs two-GPU image output parity depends on those two normalizations agreeing. Worth reusing the validated GPU top-k result instead of re-deriving it on the host, and confirming the two weighings match exactly (the 213/220 identical-answer result suggests only borderline tokens diverge).</comment>
<file context>
@@ -6694,6 +6732,21 @@ static bool eval_ds4_layer_range_hybrid_ffn(
float * token_weights =
weights.data() + (size_t)t * (size_t)route_width;
+ if (vision::image_block_at(image_spans, uint64_t(kv_start + t))) {
+ vision::ImageExpertSelection selection;
+ std::string error;
</file context>
…cipe The converter left every dense tensor in BF16 and put every down expert in fp3, which made its files 22 GB larger and slower to decode than the model we ship. It now follows the shipped recipe: dense projections and the output head in ROCmFP4, the token embedding in Q6_K, gate and up experts in fp2, and down experts in fp2 on the shipped layer set (--down-fp2-layers, default 0,2-4,6,10,11,17-20,39-42) and fp3 elsewhere. The fp2 down codebooks go to the fp2 sidecar, the fp3 table lists only the fp3 layers and is omitted when there are none. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…converter The converter took one importance vector per expert tensor, so every one of the 256 experts in a layer was weighted by the average of all of them. It now also accepts llama.cpp's per-expert layout (one vector per expert, expert major) and weights each expert by its own tokens; a single shared vector still works. The fp2 codebooks are embedded in the GGUF metadata as deepseek4.gumix.sidecar, which the loader already prefers, so the output is one file like the published DeepSeek-V4-Flash builds, with no .gumix.bin beside it. The calibration note in the metadata names the imatrix file. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/tools/ds4_mix_converter/ds4_mix_converter.cpp">
<violation number="1" location="server/tools/ds4_mix_converter/ds4_mix_converter.cpp:426">
P2: `parse_layer_set` silently accepts trailing characters in layer specifications, causing malformed `--down-fp2-layers` input to select the wrong layers. Require each `stoi` parse to consume the entire substring.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const int lo = std::stoi(item.substr(0, dash)); | ||
| const int hi = dash == std::string::npos ? lo : std::stoi(item.substr(dash + 1)); |
There was a problem hiding this comment.
P2: parse_layer_set silently accepts trailing characters in layer specifications, causing malformed --down-fp2-layers input to select the wrong layers. Require each stoi parse to consume the entire substring.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tools/ds4_mix_converter/ds4_mix_converter.cpp, line 426:
<comment>`parse_layer_set` silently accepts trailing characters in layer specifications, causing malformed `--down-fp2-layers` input to select the wrong layers. Require each `stoi` parse to consume the entire substring.</comment>
<file context>
@@ -391,6 +400,37 @@ constexpr std::array<ExpertRecipe, 3> kExpertRecipes{{
+ while (std::getline(in, item, ',')) {
+ if (item.empty()) continue;
+ const auto dash = item.find('-');
+ const int lo = std::stoi(item.substr(0, dash));
+ const int hi = dash == std::string::npos ? lo : std::stoi(item.substr(dash + 1));
+ if (lo < 0 || hi < lo) fail("bad layer range: " + item);
</file context>
| const int lo = std::stoi(item.substr(0, dash)); | |
| const int hi = dash == std::string::npos ? lo : std::stoi(item.substr(dash + 1)); | |
| auto parse_layer = [&](const std::string & value) { | |
| size_t consumed = 0; | |
| const int parsed = std::stoi(value, &consumed); | |
| if (consumed != value.size()) fail("bad layer range: " + item); | |
| return parsed; | |
| }; | |
| const int lo = parse_layer(item.substr(0, dash)); | |
| const int hi = dash == std::string::npos ? lo : parse_layer(item.substr(dash + 1)); |
…projector Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Downloads lodepng.cpp and lodepng.h from the pinned commit's raw files, checks each against its SHA256, retries three times, and re-fetches a cached file whose hash is wrong. Drops 9,464 vendored lines from the tree; the license text stays in ImageCodecs.NOTICES.md. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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/cmake/ImageCodecs.cmake">
<violation number="1" location="server/cmake/ImageCodecs.cmake:49">
P1: Create the hash-named lodepng directory before downloading; otherwise every clean default configure fails before the PNG codec target is created.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # byte-stable, so fetch its two files at a pinned commit (raw files are) and | ||
| # check each against its hash. Retried because CI runners drop downloads. | ||
| set(IMAGE_CODEC_PNG_COMMIT ed6fe5825c6a4fbb7f58ab35a4231c7543cd452a) | ||
| set(IMAGE_CODEC_PNG_DIR ${CMAKE_CURRENT_BINARY_DIR}/lodepng-${IMAGE_CODEC_PNG_COMMIT}) |
There was a problem hiding this comment.
P1: Create the hash-named lodepng directory before downloading; otherwise every clean default configure fails before the PNG codec target is created.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmake/ImageCodecs.cmake, line 49:
<comment>Create the hash-named lodepng directory before downloading; otherwise every clean default configure fails before the PNG codec target is created.</comment>
<file context>
@@ -42,8 +42,38 @@ set_target_properties(image_codec_jpeg PROPERTIES
+# byte-stable, so fetch its two files at a pinned commit (raw files are) and
+# check each against its hash. Retried because CI runners drop downloads.
+set(IMAGE_CODEC_PNG_COMMIT ed6fe5825c6a4fbb7f58ab35a4231c7543cd452a)
+set(IMAGE_CODEC_PNG_DIR ${CMAKE_CURRENT_BINARY_DIR}/lodepng-${IMAGE_CODEC_PNG_COMMIT})
+foreach(entry
+ "lodepng.cpp=d98e1f40d303c1038a096ebf93b413a565a91cf2c72b9d2fa5c625c4279c3cb6"
</file context>
| set(IMAGE_CODEC_PNG_DIR ${CMAKE_CURRENT_BINARY_DIR}/lodepng-${IMAGE_CODEC_PNG_COMMIT}) | |
| set(IMAGE_CODEC_PNG_DIR ${CMAKE_CURRENT_BINARY_DIR}/lodepng-${IMAGE_CODEC_PNG_COMMIT}) | |
| file(MAKE_DIRECTORY ${IMAGE_CODEC_PNG_DIR}) |
…OJ for Docker Download and launch commands for Qwen3.8-27B on one GPU and DeepSeek V4 Flash Vision on a Strix Halo, both pointing at the files on the Lucebox Hugging Face repos, plus a curl example that sends an image. The Docker entrypoint maps LUCE_MMPROJ to --mmproj. The README links the guide. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
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/scripts/entrypoint.sh">
<violation number="1" location="server/scripts/entrypoint.sh:504">
P3: LUCE_MMPROJ is the only file-path env var in this block that is not preflighted with `[ -f ]`. A typo or an unmounted projector file makes the server abort mid-initialization rather than failing fast with the entrypoint's clear `die` message (as LUCE_PREFILL_DRAFTER does). Guard it: `[ -f "$LUCE_MMPROJ" ] || die "Vision projector not found at $LUCE_MMPROJ"` before appending `--mmproj`.</violation>
</file>
<file name="docs/image-input.md">
<violation number="1" location="docs/image-input.md:53">
P3: This quick start presents `LUCE_DS4_SPARSE_DECODE_FLASH=1` as a plain part of the launch, but the rest of the repo documents it as an experimental single-HIP opt-in that can change generated tokens (ENVIRONMENT.md, DS4.md, RECOMMENDED_SETUPS.md all say so). Users copying the canonical snippet will silently enable a non-default, output-changing verifier path, which conflicts with the 'published launch plus --mmproj' framing. Add the same experimental/single-HIP/may-change-tokens comment used in RECOMMENDED_SETUPS.md.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| [ -n "$DRAFT_ARG" ] && CMD+=(--ddtree --ddtree-budget "$LUCE_BUDGET") | ||
| [ -n "$LUCE_DEFAULT_MAX_TOKENS" ] && CMD+=(--default-max-tokens "$LUCE_DEFAULT_MAX_TOKENS") | ||
| [ -n "$LUCE_MODEL_NAME" ] && CMD+=(--model-name "$LUCE_MODEL_NAME") | ||
| [ -n "${LUCE_MMPROJ:-}" ] && CMD+=(--mmproj "$LUCE_MMPROJ") |
There was a problem hiding this comment.
P3: LUCE_MMPROJ is the only file-path env var in this block that is not preflighted with [ -f ]. A typo or an unmounted projector file makes the server abort mid-initialization rather than failing fast with the entrypoint's clear die message (as LUCE_PREFILL_DRAFTER does). Guard it: [ -f "$LUCE_MMPROJ" ] || die "Vision projector not found at $LUCE_MMPROJ" before appending --mmproj.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/scripts/entrypoint.sh, line 504:
<comment>LUCE_MMPROJ is the only file-path env var in this block that is not preflighted with `[ -f ]`. A typo or an unmounted projector file makes the server abort mid-initialization rather than failing fast with the entrypoint's clear `die` message (as LUCE_PREFILL_DRAFTER does). Guard it: `[ -f "$LUCE_MMPROJ" ] || die "Vision projector not found at $LUCE_MMPROJ"` before appending `--mmproj`.</comment>
<file context>
@@ -501,6 +501,7 @@ CMD=("$LUCE_SERVER_BIN" "$LUCE_TARGET"
[ -n "$DRAFT_ARG" ] && CMD+=(--ddtree --ddtree-budget "$LUCE_BUDGET")
[ -n "$LUCE_DEFAULT_MAX_TOKENS" ] && CMD+=(--default-max-tokens "$LUCE_DEFAULT_MAX_TOKENS")
[ -n "$LUCE_MODEL_NAME" ] && CMD+=(--model-name "$LUCE_MODEL_NAME")
+[ -n "${LUCE_MMPROJ:-}" ] && CMD+=(--mmproj "$LUCE_MMPROJ")
# `--lazy-draft` is silently dropped by the C++ server unless both
# `--prefill-drafter` and `--draft` are present (look for the runtime
</file context>
| [ -n "${LUCE_MMPROJ:-}" ] && CMD+=(--mmproj "$LUCE_MMPROJ") | |
| if [ -n "${LUCE_MMPROJ:-}" ]; then | |
| [ -f "$LUCE_MMPROJ" ] || die "Vision projector not found at $LUCE_MMPROJ" | |
| CMD+=(--mmproj "$LUCE_MMPROJ") | |
| fi |
|
|
||
| LUCE_DS4_SPEC=1 \ | ||
| LUCE_DS4_DRAFT=models/DeepSeek-V4-Flash-0731-DSpark-draft-Q4RMFP4-denseF16.gguf \ | ||
| LUCE_DS4_SPARSE_DECODE_FLASH=1 \ |
There was a problem hiding this comment.
P3: This quick start presents LUCE_DS4_SPARSE_DECODE_FLASH=1 as a plain part of the launch, but the rest of the repo documents it as an experimental single-HIP opt-in that can change generated tokens (ENVIRONMENT.md, DS4.md, RECOMMENDED_SETUPS.md all say so). Users copying the canonical snippet will silently enable a non-default, output-changing verifier path, which conflicts with the 'published launch plus --mmproj' framing. Add the same experimental/single-HIP/may-change-tokens comment used in RECOMMENDED_SETUPS.md.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/image-input.md, line 53:
<comment>This quick start presents `LUCE_DS4_SPARSE_DECODE_FLASH=1` as a plain part of the launch, but the rest of the repo documents it as an experimental single-HIP opt-in that can change generated tokens (ENVIRONMENT.md, DS4.md, RECOMMENDED_SETUPS.md all say so). Users copying the canonical snippet will silently enable a non-default, output-changing verifier path, which conflicts with the 'published launch plus --mmproj' framing. Add the same experimental/single-HIP/may-change-tokens comment used in RECOMMENDED_SETUPS.md.</comment>
<file context>
@@ -4,15 +4,84 @@ The server accepts JPEG and PNG images through OpenAI chat completions when a
+
+LUCE_DS4_SPEC=1 \
+LUCE_DS4_DRAFT=models/DeepSeek-V4-Flash-0731-DSpark-draft-Q4RMFP4-denseF16.gguf \
+LUCE_DS4_SPARSE_DECODE_FLASH=1 \
+./server/build-hip/luce_server models/DeepSeek-V4-Flash-Vision-Exp-ROCMFPX-MIX-STRIX.gguf \
+ --target-device hip:0 --max-ctx 131072 --chunk 8192 \
</file context>
| LUCE_DS4_SPARSE_DECODE_FLASH=1 \ | |
| # LUCE_DS4_SPARSE_DECODE_FLASH=1 stays an explicit experimental opt-in | |
| # (single HIP target; may change generated tokens — see DS4.md). | |
| LUCE_DS4_SPARSE_DECODE_FLASH=1 \ |
2 participants
Summary
DeepSeek V4 Flash Vision serving on the Strix Halo + 7900 XT pair, plus a bounded IQ85 converter for the abliterated parent. 83 commits, 135 files, +14651/−59.
Vision serving
03edb38), with image policy, transport, preprocess and prompt-ownership units and a CPU composition proof.export_ds4v_mmproj.py) verified lossless: 267 tensors, 932786176 payload bytes compared against the parent by an independent reader.Serving memory, prefill and decode
Text correctness
4bf7270.Tooling
server/tools/ds4v_vision/probes and contracts,ds4v_preprocess_probe, and--recipe iq85inds4_mix_converter: IQ2_XXS gate/up, IQ2_XS down, Q8_0 for dense text matrices, vision/aligner/router/indexer/norms/biases preserved, reading the original safetensors rather than a previously quantized GGUF. Newtest_ds4_iq_converter.Verification status — gates unchanged, nothing here is a quality claim
7071946, binaryc32e5ae3); short decode 15.9/16.4/17.4 t/s.0.99822935, first native HIP run0.9859(exit 3). Tower A is the experimental base; the numerical verdict is still ISSUES and runtime integration stays blocked.954433dc…), but the frozen quality gate failed (12/16 content, 5/16 strict) and median decode was 20.1 t/s against a 35 t/s target. No candidate was installed; the original service was restored.Full receipts:
artifacts/ds4v-quant85/README.md,docs/ds4v-continuation-status.md,decisions-ds4v-continuation.tsv.Notes
Binary trial evidence (requests, responses, journals) is retained outside the repository. The converter path is experimental and, per the evidence above, does not yet meet the target quality or speed.
Upstream sync
upstream/main(2f0eff05) is merged into the branch (6d762d8e) so the PR applies cleanly again. Conflict resolutions:GGML_OP_DS4_MOE_COMBINE);GGML_OP_COUNT105 → 110 in the enum and in bothggml.ctables.compute_ds4_hybrid_budget_infonow takes bothwith_visionandpaged; it charges paged KV on the primary target and still reserves the vision scratch allowance viavision::remaining_expert_budget.build_mla_attention_lane_coretakes upstream's prepared lane andout_attn_contextalongside the image span view; the contiguous wrapper forwards image spans so existing callers keep working.deepseek4_snapshot.cpp; this branch's inline copies were byte-identical to the merge base, so they were dropped anddeepseek4_release_image_scratchwas kept.Local checks on macOS with clang (C++17, HIP headers stubbed — Apple has no HIP):
ggml.ccompiles clean and the op enum plus both tables contain 110 entries;deepseek4_graph.cpp,deepseek4_backend.cpp,deepseek4_snapshot.cpp,deepseek4_paged_cache.cpp, the image units andtest_deepseek4_unit.cppall pass-fsyntax-only. No link or GPU-run verification was possible here. Upstream CI runs for this head are inaction_required(maintainer approval pending), so the CUDA build has not executed yet.