Port Deepseek v4 - #2110
Port Deepseek v4#2110SamuelOliveirads wants to merge 34 commits into
Conversation
…oftplus gating function
|
I'll look into it. But as a general observation, I wasn't expecting a DS4 PR will add over 3k LOC. We now have the main ingredients (indexer via the GLM-DSA PRs, HC and SWA via the openPangu PR) on the main branch, so please reuse as much of that as possible (you will probably need to extract the components from |
|
Your branch is quite a bit behind the main branch. Please rebase, fix conflicts, and check what has happened on the main branch. Do not reinvent the wheel with sparse attention, HC, sinkhorn, etc. I have one of the antirez ds4 GGUFs. Mainline works with that, this PR crashes. Not sure if I want to download yet another 160 GB to test the PR and see what can be done about the issues you are seeing. Can you make it work with antirez GGUFs? OK, one issue is that the GGUF I have has the MTP layer included, and this is not taken into account when building the graph. This leads to a crash in Better luck running CPU-only. Better in the sense of no crash (after fixing the MTP issue). But running PPL values are about the same with or without In contrast, here is what I get with mainline Oh, initially I tried to run So, it looks like it simply does not work yet, and is not related to issues with IQK flash attention. |
# Conflicts: # convert_hf_to_gguf.py # ggml/src/ggml.c # src/CMakeLists.txt # src/llama-build-context.cpp # src/llama-build-context.h # src/llama-hparams.cpp # src/llama-load-tensors.cpp
… post-processing with new helper functions
|
The problem with the Antirez GGUF arose mainly because I make use of some of the GLM_DSA’s logic and as a result relied on the generic management of
With
As for CUDA non-FA:
Note that for perplexity I used Regarding the rebase, I was going to wait until the logic was working, but now that you sugested I did it now to reuse what had already been done for HC, SWA, Indexer, and Sinkhorn, hopefully I was able to reuse everything that made sense. Back to FA for CPU, the perplexity is similar but the token selection differs from the mainline. This is most striking in CUDA where |
|
On the latest PR version, CPU-only This after fixing the build error due to the duplicate instantiation of - for (int il = 0; il < n_layer; ++il) {
+ int n_active_layer = n_layer - hparams.nextn_predict_layers;
+
+ for (int il = 0; il < n_active_layer; ++il) {Another try CUDA (with
|
| 0); | ||
| } | ||
|
|
||
| // Keep DSV4 normalization explicit. The generic helper may fuse RMS |
There was a problem hiding this comment.
And this is required because? The generic llm_build_norm is used in absolutely all models with absolutely zero issues, but suddenly DS4 requires that the RMS norm is not fused?
There was a problem hiding this comment.
At one point during the port, using the mainline RMS helped keep the output consistent. To be fair I ran a test with norm again and didn't encounter any problems so I imagine it was an unintended side effect during development.
| return cur; | ||
| } | ||
|
|
||
| static ggml_tensor * dsv4_build_raw_k_from_idxs( |
| dsv4_new_i64_input(ctx, &inputs.state_write_idxs, (int64_t) plan.state_write_idxs.size(), (std::string(tag) + "_state_write").c_str()); | ||
| dsv4_new_i32_input(ctx, &inputs.state_write_pos, (int64_t) plan.state_write_pos.size(), (std::string(tag) + "_write_pos").c_str()); | ||
| dsv4_new_mask_input(ctx, &inputs.kq_mask, std::max<int64_t>(1, plan.n_kv), n_tokens, (std::string(tag) + "_kq_mask").c_str()); | ||
| if (std::strcmp(tag, "dsv4_lid") == 0) { |
There was a problem hiding this comment.
llama.cpp style nonsense is not tolerated in this repository. We do have ggml_hadamard, and this is what you need to use.
There was a problem hiding this comment.
Oh, that's right! My bad, the plan was to test their code and then adapt it to what we have here but I ended up forgetting to get to that part.
| const int64_t n_indexer_head = hparams.indexer_n_head; | ||
| const int64_t n_tokens = cur->ne[1]; | ||
| const int64_t n_lid = llm.lctx.dsv4.lid_plan.n_kv; | ||
| ggml_tensor * k_rot = llm.lctx.dsv4.inputs.lid.k_rot; |
| hparams.dsv4_compress_rope_base, llm.freq_scale, | ||
| llm.ext_factor, dsv4_rope_attn_factor(llm.freq_scale, llm.ext_factor), llm.beta_fast, llm.beta_slow); | ||
| indexer_q = ggml_concat(ctx0, indexer_q_nope, indexer_q_pe, 0); | ||
| indexer_q = ggml_mul_mat(ctx0, k_rot, indexer_q); |
| hparams.indexer_head_size, | ||
| il, | ||
| "lid_state_compress"); | ||
| if (lctx.dsv4.inputs.lid.k_rot != nullptr) { |
| struct ggml_tensor * state_write_idxs = nullptr; | ||
| struct ggml_tensor * state_write_pos = nullptr; | ||
| struct ggml_tensor * kq_mask = nullptr; | ||
| struct ggml_tensor * k_rot = nullptr; |
| std::vector<float> csa_mask_data; | ||
| std::vector<float> hca_mask_data; | ||
| std::vector<float> lid_mask_data; | ||
| std::vector<float> lid_k_rot_data; |
| return *dst; | ||
| } | ||
|
|
||
| static int64_t dsv4_k_rot_size(int64_t n_embd) { |
There was a problem hiding this comment.
Discard and see how the Hadamard block size is determined in the standard ik_llama.cpp Hadamard transform usage.
| llm.cb(indexer_k, "lid_k_stream", il); | ||
|
|
||
| ggml_tensor * lid_mask = dsv4_build_raw_mask_view(ctx0, | ||
| llm.lctx.dsv4.inputs.lid.kq_mask, n_lid, n_tokens); |
There was a problem hiding this comment.
In what way is lid.kq_mask different from the standard attention mask that we have been building forever?
There was a problem hiding this comment.
At first glance it's semantically similar, the only difference is in the structure of the raw attention mask since the key axis is the compressed LID cache. I reused inputs.csa.kq_mask and removed duplicate inputs in the latest commits.
| return dsv4_concat_named(ctx, t, row, 1, "dsv4_append_zero_row"); | ||
| } | ||
|
|
||
| static ggml_tensor * dsv4_with_zero_dep(ggml_context * ctx, ggml_tensor * t, ggml_tensor * dep) { |
There was a problem hiding this comment.
What is this supposed to do?
We sum the elements of dep, to create tensor zero that has 1 element.
We then scale the result stored in zero with 0, to obtain a tensor containing exactly one zero.
We then use ggml_add1 to add this zero to the elements of t.
So, basically, we are wasting a bunch of CPU or GPU cycles to obtain the exact same tensor as we had before? Is this the new way of making a copy of a tensor?
There was a problem hiding this comment.
I added a comment above the function for future reference, but in short this forces compression/cache work to complete before state persistence or dependent cache reads can execute.
The graph would originally have had this dependency:
dep -> compression/cache write
t -> state persistence or dependent cache read
Since the branches are independent, we need to create a tensor dependency:
dep -> sum -> scale(0) -> add1(t, zero) -> state persistence/read
\________________________________________/
The output is numerically equal to t but it now depends on dep.
There was a problem hiding this comment.
So, forever we were able to make sure that writing to the KV cache has completed before using it, but now suddenly we cannot?
Just put
if (hca_dep) {
ggml_build_forward_expand(gf, hca_dep);
}before
ggml_tensor * hca_persist_kv = ggml_get_rows(ctx0, hca_state_kv, lctx.dsv4.inputs.hca.state_persist_src_idxs);discard all occurrences of dsv4_with_zero_dep, and lets see how that goes.
| if (raw_mask->type != hca_mask->type) { | ||
| raw_mask = ggml_cast(ctx0, raw_mask, hca_mask->type); | ||
| } | ||
| ggml_tensor * k_all = dsv4_concat_named(ctx0, raw_k, hca_k, 2, "dsv4_raw_plus_hca_k"); |
There was a problem hiding this comment.
Similar comment to the above. Why on Earth are we concatenating the regular cache/mask with the HC cache/mask?
|
|
||
| #include "concat.cuh" | ||
|
|
||
| // contiguous kernels |
There was a problem hiding this comment.
Why are changes being made to concat.cu? If the reason is that it was not the same as concat.cu in llama.cpp, that is not a valid reason to copy/paste from there. The concat ops here can serve different purposes not exercised in mainline.
There was a problem hiding this comment.
In the first version I wrote I was unable to run it using non-FA CUDA so the change was based on the assumption that DSV4 required a more generic F16/BF16 strided concatenation path. However, after making the most recent changes I ran new tests and the same problem did not occur so I reverted the code.
| return result; | ||
| } | ||
|
|
||
| struct ggml_tensor * ggml_rope_ext_back( |
There was a problem hiding this comment.
We absolutely do not need this function here. We have ggml_rope_back instead.
There was a problem hiding this comment.
I probably copied this from the mainline port.
Reverted.
| float beta_fast, | ||
| float beta_slow); | ||
|
|
||
| GGML_API struct ggml_tensor * ggml_rope_ext_back( |
There was a problem hiding this comment.
Discard this and use ggml_rope_back instead.
| ggml_tensor * normed = ggml_rms_norm(ctx0, flat, hparams.f_norm_rms_eps); | ||
| normed = ggml_mul(ctx0, normed, gamma); // [S*H, T] | ||
| ggml_tensor * mixes = ggml_mul_mat(ctx0, phi, normed); // [(S+2)*S, T] | ||
| ggml_tensor * mixes = build_mhc_pre_projection(Rin, phi, gamma, |
There was a problem hiding this comment.
It is obviously useful to try to unify DS4 and OpenPangu as there is basically the same thing being done. But after merging #2132, this change results in a ~10% performance penalty for openPangu compared to the main branch. I didn't investigate if it is the combined effect of build_mhc_pre_projection, build_mhc_weighted_sum and build_mhc_post or if it is one of those causing the performance penalty.
There was a problem hiding this comment.
From what I could tell by comparing it with main the regression occurs in build_mhc_post. I added conditions to use ggml_sum_rows_ext via OpenPangu, while DSV4 uses the old approach and this restored performance.
|
Will continue in a ~2-3 days. |
| int64_t n_stream) { | ||
| const int64_t n_tokens = x->ne[2]; | ||
| ggml_tensor * out = nullptr; | ||
|
|
There was a problem hiding this comment.
This function assumes that x->ne[3] == 1 and weights->ne[3] == 1. Also, n_stream is not actually necessary as argument because n_stream must be the same as x->ne[1] and weights->ne[0], else it will fail. Also, n_embd must be the same as x->ne[0], no?
Given this, the implementation can be replaced with
auto w = ggml_reshape_4d(ctx0, weights, 1, weights->ne[0], weights->ne[1], weights->ne[2]);
return ggml_mul_multi_add(ctx0, x, w);If we are worried that the above conditions are not met, we can say
if (x->ne[3] == 1 && weights->ne[3] == 1 && x->ne[0] == n_embd && x->ne[1] == n_stream &&
x->ne[1] == weights->ne[0] && x->ne[2] == weights->ne[1] && weights->ne[2] == 1) {
auto w = ggml_reshape_4d(ctx0, weights, 1, weights->ne[0], weights->ne[1], weights->ne[2]);
return ggml_mul_multi_add(ctx0, x, w);
}It is not a big deal, it gives just 1-2% performance boost. But still, why wouldn't we use the ik_llama.cpp abilities to their full potential?
The ggml_mul_multi_add op was specifically added to sum up the contributions of the selected experts multiplied with the experts weights. These are not experts, but the operations being performed are exactly the same.
There was a problem hiding this comment.
Good idea, I applied ggml_mul_multi_add directly and no longer materializes weights. The retained dimensions are used only to assert the shared DSV4/OpenPangu caller contract. The reshape produces [1, streams, tokens, 1], equivalent to the proposed 4D representation. I also made the ne[3] and source weight dimensions explicit so an incompatible caller fails at the helper boundary.
|
|
||
| *comb = ggml_cont(ctx0, dsv4_view_2d(ctx0, mixes, hc*hc, nt, 2*hc)); | ||
| *comb = dsv4_hc_affine(ctx0, *comb, scale_comb, base_comb); | ||
| *comb = ggml_reshape_3d(ctx0, *comb, hc, hc, nt); |
There was a problem hiding this comment.
Replace this and the next line with
*comb = ggml_sinkhorn(ctx0, *comb, hc, hparams.dsv4_hc_sinkhorn_iters, hparams.dsv4_hc_eps, false);Then remove the build_hc_sinkhorn function.
There was a problem hiding this comment.
I tested the idea, on the CPU non-FA IQ2XXS c2048 one-chunk control, the existing DSV4 path produced 3.3172 PPL, while the direct ggml_sinkhorn replacement produced 4.5795. An earlier bounded test also changed the selected token so I reverted the experiment. This is not an FA artifact, but I have not yet determined whether the mismatch is the caller’s flattened matrix orientation or the shared operation’s expected layout so I didn't applied the private path for now.
# Conflicts: # src/graphs/build_openpangu.cpp
I fixed the CPU FA mask assertion in commit abf5c96. On the Antirez IQ2XXS model at c4096, branch non-FA was Tensor probing found a very small initial branch/mainline difference at layer-0 normalization/latent projection input which is amplified by the quantized Q/K path. The fused-MoE path is a measurable contributor, but the sampled tensors do not yet prove that its kernel is the original owner so a same-input fused-expert replay is still required. |
|
Things are moving in the wrong direction. I observe a massive TG performance drop (~50%) compared to when I last checked (I believe commit 3d6acd3) with the few optimizations that I proposed added on top. That's running on 2x3090+Ryzen-3995WX with This PR
PR #2147
|
| return dsv4_build_mask_stream_view(ctx, base, std::max<int64_t>(1, n_stream), n_tokens); | ||
| } | ||
|
|
||
| ggml_tensor * mask_t = ggml_cont(ctx, ggml_transpose(ctx, mask)); |
There was a problem hiding this comment.
Can you explain what this is supposed to accomplish and why we need it?
There was a problem hiding this comment.
raw K is gathered by cache-slot indices while the original mask is position-oriented. The transpose lets ggml_get_rows() apply the identical row permutation to the mask, and ggml_cont() materializes the non-contiguous transpose for correct strides. Mainline has a specific style of raw/SWA cache, where K and the mask are generated in the same bounded order, so they don't apply this there.
|
Does anyone know why AVX-512 MXFP4 CPU-only token generation is slower than |
|
@cora4 can you give a config/invocation example? btw there is an Edit feature, it would avoid sending 4 email notices that are sent from new/delete pairs. |
|
Please do not misuse this PR for conversations about totally unrelated stuff. |
Apologies
It is related.
|
|
You can run llama-quantize --imatrix imatrix.dat --allow-requantize --token-embedding-type q8_0 --attn-k-type mxfp4 --attn-v-type mxfp4 --custom-q ffn_down_exps=mxfp4 --ffn-down-type mxfp4 model-name-Q8_0.gguf model-name-MXFP4.gguf mxfp4I have also tested it with smaller models |
|
This PR is very much WIP. You can try #2147 instead. I don't have an MXP4 version of DS4, but for GPT-OSS-20B-MXFP4 on my Ryzen-7950X CPU I observe the following llama.cpp
ik_llama.cpp
|
4 participants
This implementation is based on the mainline PR and uses DeepSeek-V4-Flash-MXFP4 for validation.
The main reason I created a draft is because the output diverges from the mainline when FA is enabled. For testing purposes, I disabled the IQK FA using the
GGML_FLASH_ATTN_EXT_IQK_DISABLEDflag, which aligns the model’s output with the mainline, however, the same issue occurs when using CUDA MMA FA path, which produces finite but incorrect logits for the DSV4 512/512/512, GQA 64:1 case, and here it’s a bit more complicated to disable the current implementation for testing. Since I don’t know exactly what to adjust in the current FA implementation in the engine, @ikawrakow I’d like to know the best course of action. I can disable FA for now so it can be fixed in the future, in that case I would revert the main changes made to ggml, concat.cu andGGML_FLASH_ATTN_EXT_IQK_DISABLEDpath.As for performance, it’s a work in progress, partly because I can’t achieve a complete offload to the GPU, so I haven’t run a full benchmark yet. Using sweep-bench with a humble -c 256 -n 32 I’ve observed the following performance:
3.83 tok/s.4.00 tok/s.2.59 tok/s3.20 tok/sThere are other limitations I haven’t addressed yet in this PR, such as context shift, quantized KV,
-sm graph, and low GPU performance, for example the routing ofl_outbut I believe they may come up later.