feat(dflash): add DeepSeek V4 Flash backend#353
Merged
Conversation
Contributor
Author
|
@davidmroth help me check if llama.cpp submodule update is needed. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
16 issues found across 39 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/deepseek4/deepseek4_loader.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_loader.cpp:376">
P2: Failure paths after `out.ctx = meta_ctx` leave `DeepSeek4Weights` in a partially initialized state without resetting `out.ctx`; the `meta_ctx` context leaks if the caller does not invoke `free_deepseek4_weights()`. Either move `out.ctx = meta_ctx` to after all error paths or reset it (and free `meta_ctx`) in each failure branch.</violation>
<violation number="2" location="server/src/deepseek4/deepseek4_loader.cpp:438">
P1: Missing mmap bounds validation before tensor copy from GGUF file data (main load path).</violation>
<violation number="3" location="server/src/deepseek4/deepseek4_loader.cpp:532">
P2: Missing mmap bounds validation in embedder byte copy.</violation>
<violation number="4" location="server/src/deepseek4/deepseek4_loader.cpp:535">
P1: Embedding-table reload failure is silently ignored, allowing model load to succeed with zeroed embeddings that corrupt all token outputs.</violation>
<violation number="5" location="server/src/deepseek4/deepseek4_loader.cpp:539">
P2: Potential divide-by-zero: `n_vocab` from GGUF metadata is not validated to be non-zero before dividing `a.file_size` by it. A malformed file with `vocab_size=0` will crash the loader.</violation>
<violation number="6" location="server/src/deepseek4/deepseek4_loader.cpp:633">
P1: Integer overflow in tensor bounds check allows malformed GGUF metadata to bypass validation</violation>
</file>
<file name="server/CMakeLists.txt">
<violation number="1" location="server/CMakeLists.txt:570">
P2: Duplicate GPU runtime link configuration for dflash_common. An identical conditional block for linking CUDA::cudart / hip::host already exists earlier in this file (around line 369).</violation>
</file>
<file name="server/src/deepseek4/deepseek4_backend.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_backend.cpp:690">
P1: park()/unpark() are no-op stubs that do not release or reacquire GPU resources, breaking the backend lifecycle contract</violation>
<violation number="2" location="server/src/deepseek4/deepseek4_backend.cpp:824">
P2: Hard-coded EOS token IDs (151643, 151644) make termination behavior fragile across tokenizer/model variants. Replace with tokenizer metadata lookup.</violation>
</file>
<file name="server/src/server/chat_template.cpp">
<violation number="1" location="server/src/server/chat_template.cpp:370">
P1: `add_generation_prompt` is gated by `pending_assistant`, so system-only or empty chats fail to emit an assistant generation prefix. All other formats in this file (QWEN3, LAGUNA, GEMMA4) check `add_generation_prompt` unconditionally.</violation>
</file>
<file name="server/src/common/moe_hybrid_ffn_eval.h">
<violation number="1" location="server/src/common/moe_hybrid_ffn_eval.h:202">
P1: New default parameter inserted in middle of `build_cached_hot_graph` signature silently remaps existing positional arguments at call site</violation>
</file>
<file name="server/src/ipc/backend_ipc_main.cpp">
<violation number="1" location="server/src/ipc/backend_ipc_main.cpp:128">
P2: fprintf format/argument mismatch: 6 `%s` placeholders but only 5 `argv[0]` arguments supplied, causing undefined behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
Author
|
notice 20% regression running 27B dense model. |
howard0su
force-pushed
the
ds4
branch
2 times, most recently
from
dc41ed2 to
8d11f77
Compare
Contributor
There was a problem hiding this comment.
17 issues found across 46 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/deepseek4/deepseek4_loader.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_loader.cpp:108">
P2: The `get_u32_arr` helper reads array data as `int32_t` without first checking the array element type via `gguf_get_arr_type`. If the GGUF stores the key as a non-32-bit array type (e.g., INT64 or UINT64), the element indexing will read wrong offsets and corrupt hyperparameters like `compress_ratios`. Other loaders in this repo already verify the element type before casting `gguf_get_arr_data` (see `laguna_target_loader.cpp` and `draft_gguf_loader.cpp`); adding the same guard here aligns with codebase conventions and prevents schema-evolution bugs.</violation>
<violation number="2" location="server/src/deepseek4/deepseek4_loader.cpp:376">
P2: Failure paths after `out.ctx = meta_ctx` leave `DeepSeek4Weights` in a partially initialized state without resetting `out.ctx`; the `meta_ctx` context leaks if the caller does not invoke `free_deepseek4_weights()`. Either move `out.ctx = meta_ctx` to after all error paths or reset it (and free `meta_ctx`) in each failure branch.</violation>
<violation number="3" location="server/src/deepseek4/deepseek4_loader.cpp:438">
P1: Missing mmap bounds validation before tensor copy from GGUF file data (main load path).</violation>
<violation number="4" location="server/src/deepseek4/deepseek4_loader.cpp:532">
P2: Missing mmap bounds validation in embedder byte copy.</violation>
<violation number="5" location="server/src/deepseek4/deepseek4_loader.cpp:539">
P2: Potential divide-by-zero: `n_vocab` from GGUF metadata is not validated to be non-zero before dividing `a.file_size` by it. A malformed file with `vocab_size=0` will crash the loader.</violation>
<violation number="6" location="server/src/deepseek4/deepseek4_loader.cpp:633">
P1: Integer overflow in tensor bounds check allows malformed GGUF metadata to bypass validation</violation>
</file>
<file name="server/CMakeLists.txt">
<violation number="1" location="server/CMakeLists.txt:570">
P2: Duplicate GPU runtime link configuration for dflash_common. An identical conditional block for linking CUDA::cudart / hip::host already exists earlier in this file (around line 369).</violation>
</file>
<file name="server/src/deepseek4/deepseek4_backend.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_backend.cpp:621">
P1: EOS detection in `do_decode` uses hardcoded token IDs (151643 and 151644). Cross-file evidence confirms 151644 is actually `<|im_start|>`, a role marker, not an EOS token (see `server/tests/test_tokenizer.py`). The qwen3 backend correctly uses 151645 for EOT. If a DS4 GGUF variant uses different EOS/EOT IDs — or if the model emits an im_start token during chat-formatted decode — generation will prematurely stop or over-generate. Add `eos_token_id` / `eot_token_id` fields to `DeepSeek4Weights` and read them from the loaded GGUF tokenizer metadata instead of hardcoding these constants.</violation>
<violation number="2" location="server/src/deepseek4/deepseek4_backend.cpp:690">
P1: park()/unpark() are no-op stubs that do not release or reacquire GPU resources, breaking the backend lifecycle contract</violation>
<violation number="3" location="server/src/deepseek4/deepseek4_backend.cpp:824">
P2: Hard-coded EOS token IDs (151643, 151644) make termination behavior fragile across tokenizer/model variants. Replace with tokenizer metadata lookup.</violation>
</file>
<file name="server/src/server/chat_template.cpp">
<violation number="1" location="server/src/server/chat_template.cpp:370">
P1: `add_generation_prompt` is gated by `pending_assistant`, so system-only or empty chats fail to emit an assistant generation prefix. All other formats in this file (QWEN3, LAGUNA, GEMMA4) check `add_generation_prompt` unconditionally.</violation>
</file>
<file name="server/src/common/moe_hybrid_ffn_eval.h">
<violation number="1" location="server/src/common/moe_hybrid_ffn_eval.h:202">
P1: New default parameter inserted in middle of `build_cached_hot_graph` signature silently remaps existing positional arguments at call site</violation>
</file>
<file name="server/src/ipc/backend_ipc_main.cpp">
<violation number="1" location="server/src/ipc/backend_ipc_main.cpp:128">
P2: fprintf format/argument mismatch: 6 `%s` placeholders but only 5 `argv[0]` arguments supplied, causing undefined behavior.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_internal.h">
<violation number="1" location="server/src/deepseek4/deepseek4_internal.h:187">
P2: The `DeepSeek4Weights` struct initializes all architecture-critical dimensions with hard-coded defaults (n_layer=43, n_embd=4096, n_vocab=129280, etc.). The loader (`deepseek4_loader.cpp`) uses `get_u32_or` / `get_f32_or` / `get_u64_or` helpers that silently fall back to these defaults when GGUF metadata keys are missing or renamed. This means a malformed or incompatible GGUF file can load "successfully" with incorrect dimensions, leading to subtle silent miscomputations or memory corruption later instead of an early hard failure. Consider either removing defaults from the struct and requiring the loader to populate every field, or adding mandatory-key validation so the loader rejects GGUFs with missing required metadata.</violation>
<violation number="2" location="server/src/deepseek4/deepseek4_internal.h:361">
P2: The `DeepSeek4Snapshot` schema omits per-layer compressor rolling state (`attn_compressor` and `indexer_compressor` with their `state_kv`/`state_score` tensors), which is present in `DeepSeek4LayerCache`. If snapshot save/restore is implemented against this struct, resuming after a snapshot would reconstruct raw and compressed KV tensors but not the rolling compressor buffers, causing subsequent KV compression outputs to diverge from pre-snapshot execution and producing non-deterministic attention logits even with an identical token history. Consider adding `DeepSeek4CompressorState attn_compressor` and `indexer_compressor` fields to `LayerSnap` (or equivalent tensor copies) so the eventual save/restore TODO can capture full cache fidelity.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_expert_ipc_daemon.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_expert_ipc_daemon.cpp:338">
P2: `read_eval_request` allocates vectors based on unchecked `int32_t` header dimensions before any model-bounds validation, so a malformed request file can force huge allocations or integer-overflow-driven reads. The caller later checks `hdr.n_embd != worker.weights.n_embd`, but only after the vectors are already allocated. Consider adding upper-bound checks (e.g., `n_tokens` ceiling, `n_selected <= n_expert`) and an overflow-safe multiplication check before `std::vector::assign`, or perform the model-bounds validation before allocating.</violation>
</file>
<file name="server/src/common/expert_ipc.cpp">
<violation number="1" location="server/src/common/expert_ipc.cpp:143">
P2: If the worker pipe breaks (worker crash, `EPIPE`), the unchecked `fprintf`/`fflush` in `eval_begin()` fails silently, yet `pending.active` is still set to `true` and the function returns `true`. The caller then enters `eval_end()` and blocks forever on `read_exact_fd`, which has no timeout. Please check the return values of the control-channel write and `fflush` so a broken pipe fails fast instead of hanging. The same unchecked-write problem exists in `ping()`.</violation>
<violation number="2" location="server/src/common/expert_ipc.cpp:162">
P2: The early-return guard in `eval_end` skips cleanup of the pending request file and pending state. If `eval_begin` succeeds but the client is closed or the process crashes before `eval_end` completes, the guard returns `false` without deleting `pending.path` or resetting `pending.active`, leaking temp files and leaving stale state. Consider moving `std::remove(pending.path.c_str())` and `pending.active = false` to a scope guard or handling them before the early return.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_target_shard_ipc_daemon.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_target_shard_ipc_daemon.cpp:175">
P2: The argmax loop iterates up to `weights.n_vocab` while using `resp.logits.data()` as a raw pointer, but never checks that the vector actually contains `n_vocab` elements. If `deepseek4_step_layer_range` ever returns a differently-sized logits vector, this will read out of bounds. Add a size guard before indexing.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_layer_split_adapter.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_layer_split_adapter.cpp:377">
P1: In `init_mixed_target_split_full()`, `remote_layer_begins.front()` and `remote_layer_ends.back()` are dereferenced unconditionally after a loop that may leave these vectors empty. When `layer_split_gpus` has only one element (which can happen when `cuda_layers >= 43` in the caller's auto-split logic), this produces undefined behavior and will typically crash. Add a size guard before building `launch_cfg` and before logging, or reject the mixed-target path when there are no remote layers to offload.</violation>
<violation number="2" location="server/src/deepseek4/deepseek4_layer_split_adapter.cpp:615">
P1: The `snapshot_save` and `snapshot_restore` methods persist request position and HC state, but they omit the per-shard KV/compression caches (`shard.cache`). After a restore, `cur_pos_` is rewound while the caches retain state from whatever ran most recently, so subsequent `run_forward` calls will compute attention over an inconsistent history and produce incorrect logits/tokens. The `snapshot_backends_` infrastructure is initialized and torn down but never used in the save/restore path, so add per-shard cache serialization (via the existing snapshot backends or equivalent) to keep prefix state consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (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/src/deepseek4/deepseek4_internal.h">
<violation number="1" location="server/src/deepseek4/deepseek4_internal.h:187">
P2: The `DeepSeek4Weights` struct initializes all architecture-critical dimensions with hard-coded defaults (n_layer=43, n_embd=4096, n_vocab=129280, etc.). The loader (`deepseek4_loader.cpp`) uses `get_u32_or` / `get_f32_or` / `get_u64_or` helpers that silently fall back to these defaults when GGUF metadata keys are missing or renamed. This means a malformed or incompatible GGUF file can load "successfully" with incorrect dimensions, leading to subtle silent miscomputations or memory corruption later instead of an early hard failure. Consider either removing defaults from the struct and requiring the loader to populate every field, or adding mandatory-key validation so the loader rejects GGUFs with missing required metadata.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_backend.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_backend.cpp:621">
P1: EOS detection in `do_decode` uses hardcoded token IDs (151643 and 151644). Cross-file evidence confirms 151644 is actually `<|im_start|>`, a role marker, not an EOS token (see `server/tests/test_tokenizer.py`). The qwen3 backend correctly uses 151645 for EOT. If a DS4 GGUF variant uses different EOS/EOT IDs — or if the model emits an im_start token during chat-formatted decode — generation will prematurely stop or over-generate. Add `eos_token_id` / `eot_token_id` fields to `DeepSeek4Weights` and read them from the loaded GGUF tokenizer metadata instead of hardcoding these constants.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
This was referenced Jul 5, 2026
Contributor
There was a problem hiding this comment.
1 issue found across 13 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/src/deepseek4/deepseek4_loader.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_loader.cpp:737">
P2: The bounds-validation and tensor-lookup logic is duplicated almost verbatim across `build_deepseek4_moe_hybrid_storage_from_file` and `build_deepseek4_moe_hybrid_storage_from_file_with_mmap`. Any future fix to the validation (e.g., adding new checks, adjusting error formatting, or correcting an edge case) would need to be applied in both places, raising the risk of divergence. Consider extracting the GGUF open + mmap + tensor-finding loop into a shared helper that returns `layer_file_data`, leaving each caller to only differ in the final `build_moe_hybrid_storage_*` invocation and cleanup.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const size_t data_start = gguf_get_data_offset(gctx); | ||
| const auto * file_bytes = static_cast<const uint8_t *>(mmap.addr); | ||
| std::vector<LayerExpertFileData> layer_file_data((size_t)w.n_layer); | ||
| bool bad_bounds = false; |
Contributor
There was a problem hiding this comment.
P2: The bounds-validation and tensor-lookup logic is duplicated almost verbatim across build_deepseek4_moe_hybrid_storage_from_file and build_deepseek4_moe_hybrid_storage_from_file_with_mmap. Any future fix to the validation (e.g., adding new checks, adjusting error formatting, or correcting an edge case) would need to be applied in both places, raising the risk of divergence. Consider extracting the GGUF open + mmap + tensor-finding loop into a shared helper that returns layer_file_data, leaving each caller to only differ in the final build_moe_hybrid_storage_* invocation and cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_loader.cpp, line 737:
<comment>The bounds-validation and tensor-lookup logic is duplicated almost verbatim across `build_deepseek4_moe_hybrid_storage_from_file` and `build_deepseek4_moe_hybrid_storage_from_file_with_mmap`. Any future fix to the validation (e.g., adding new checks, adjusting error formatting, or correcting an edge case) would need to be applied in both places, raising the risk of divergence. Consider extracting the GGUF open + mmap + tensor-finding loop into a shared helper that returns `layer_file_data`, leaving each caller to only differ in the final `build_moe_hybrid_storage_*` invocation and cleanup.</comment>
<file context>
@@ -627,22 +734,38 @@ bool build_deepseek4_moe_hybrid_storage_from_file(
const size_t data_start = gguf_get_data_offset(gctx);
const auto * file_bytes = static_cast<const uint8_t *>(mmap.addr);
std::vector<LayerExpertFileData> layer_file_data((size_t)w.n_layer);
+ bool bad_bounds = false;
+ std::string bounds_err;
</file context>
Implement full DS4 Flash model backend for AR-only decode: - deepseek4_internal.h: data structures (layer, weights, cache, config) - deepseek4_loader.cpp: GGUF loader with all DS4 metadata/tensor binding - deepseek4_graph.cpp: ggml compute graph (MLA attention, KV compression with ratio-4/ratio-128, indexer selective attention, MoE with sqrt(softplus) routing, hash routing, HC residual streams) - deepseek4_backend.cpp: ModelBackend subclass with hybrid hot/cold expert placement (DFLASH_DS4_HYBRID=1) - deepseek4_daemon.cpp: daemon entry point Integration: - Register 'deepseek4' arch in backend_factory.cpp - Add to CMakeLists.txt (include path + sources) Tests: - test_deepseek4_unit.cpp: CPU-only unit tests with synthetic weights (compressor pooling, MoE routing, RMSNorm, grouped output shape, hash routing lookup) - deepseek4-vectors/: official API test vectors ported from ds4 project (greedy decode logprob fixtures for integration testing)
The DS4 Flash GGUF stores rope.scaling.original_context_length as u32 and compress_ratios as i32 array. Handle both type widths gracefully.
The previous approach set dst->data directly but didn't associate the tensor with its backend buffer, causing 'tensor buffer not set' assert. Now uses ggml_backend_tensor_alloc (matching qwen35 loader pattern). Also keeps token_embd on CPU for embedding lookup.
TargetLoadPlan.layer_end defaults to -1 (not 0), so check for < 0.
When full model load fails (e.g., 81GB model on 24GB GPU), automatically fall back to hybrid mode (experts on CPU, core on GPU).
…er shapes - Output projection now correctly uses batched 3D matmul for grouped low-rank: reshape out_a [4096,8192] to [4096,1024,8], reshape q to [4096,8,n_tok], batched matmul → [1024,8,n_tok] → out_b [8192,4096] - Attention placeholder: use reshaped q (correct shape [32768,n_tok]) instead of broken kv×q matmul - Disable compressed context block (shapes incompatible with placeholder)
HC build_hc_pre returns [n_embd] (1D) but the graph expects [n_embd, n_tokens]. Bypass HC entirely until proper multi-token HC state management is implemented.
The 3D matmul batch dimension (ne[2]) must match between weight and input. Use permute to put n_out_group in ne[2] for both tensors so ggml can broadcast correctly across the group dimension.
Ratio-4 layers use comp_width = 2*head_dim (1024) with 2*ratio state rows. Ratio-128 layers use comp_width = head_dim (512). Indexer uses n_indexer_head_dim (128) as output, not full multi-head width. Pooling placeholder just takes first head_dim elements for now.
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
2 participants
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.
Implement full DS4 Flash model backend for AR-only decode:
Integration:
Tests: