Fix eager experts for DeepSeekV2 - #43288
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
thanks @Rocketknight1 i was surprised as well when writing batched_mm and grouped_mm that top_k_weights can have two possible shapes while the eager path seemed very standardized across MoEs. so i added support for both shapes without touching the eager path (assuming that it works fine somehow 😅). |
|
Yeah, I'm not sure how this ever worked! I wonder how long this bug has existed for |
|
not for long, i see it was introduced in #42456 |
|
cc @ydshieh more flaky tests here, I think possibly Hub issues again? |
|
run-slow: deepseek_v2 |
|
This comment contains models: ["models/deepseek_v2"] |
CI Results✅ No failing test specific to this PR 🎉 ! |
|
@IlyasMoutawwakil tests are green! Can you approve it so I can merge? |
| super().__init__(config) | ||
| self.num_experts = config.n_routed_experts | ||
|
|
||
| def forward( |
There was a problem hiding this comment.
Sorry I have to say that this is likely the wrong fix and will make things incompatible with things like fp8, we should change the gates instead.
Essentially, this scatter changes the shapes
We should simply remove that line and everything else should be resolved then (without these changes here)
There was a problem hiding this comment.
good catch ! standardizing the router's outputs would definitely make things easier
There was a problem hiding this comment.
Yes, agreed! I wasn't too familiar with the code and your fix seems like a better one
There was a problem hiding this comment.
I don't understand how the change breaks FP8 compatibility, though? If you look at the modeling file, it only updates one slice
There was a problem hiding this comment.
Because the weights are exchanged as is and the forward needs to be overwritten --> it has to expect the same input as the base version in the modeling code. FP8 currently assumes the version where we do not use the scatter
Reference:
transformers/src/transformers/integrations/finegrained_fp8.py
Lines 530 to 563 in 38e5987
There was a problem hiding this comment.
so with this deepseekv2 will have its top_k_weights with shape (num_tokens, num_top_k) ? like mixtral, qwen moe, etc
|
[For maintainers] Suggested jobs to run (before merge) run-slow: deepseek_v2 |
|
run-slow: deepseek_v2 |
|
This comment contains models: ["models/deepseek_v2"] |
|
@vasqu confirmed in local testing that your fix also recovers the correct output! |
CI Results✅ No failing test specific to this PR 🎉 ! |
* Fix eager experts for DeepSeekV2 * Use @vasqu's fix instead * Propagate modular
The eager expert computation for DeepSeekV2 inherited from Qwen2MoE, but they pass expert weights differently. Qwen2MoE selects experts before
forward(), it passestop_k_weightswith shape(num_tokens, num_top_k). DeepSeekV2 does not, sotop_k_weightshas shape(num_tokens, num_experts). The result is that DeepSeekV2 selected incorrect experts when_experts_implementation == "eager"and the output was garbage.This PR overrides the
forward()method for DeepSeek so we select the right experts!cc @IlyasMoutawwakil, fixes #43224