-
Notifications
You must be signed in to change notification settings - Fork 532
[MM][Bugfix] Add MoE verification for multi-modal models #3897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes a bug in is_moe_model to correctly identify multi-modal MoE models. The change introduces a check for text_config in multi-modal model configurations. While the overall logic is sound, I've identified a couple of areas for improvement. Firstly, a new global variable is modified within a function, which should be a module-level constant. Secondly, the method for detecting MoE models can be made more precise and efficient by directly checking for the num_experts key, as mentioned in the PR description, instead of using a substring search. My review comments provide specific suggestions to address these points.
Signed-off-by: shen-shanshan <467638484@qq.com>
c1d1b74 to
fb67914
Compare
Signed-off-by: shen-shanshan <467638484@qq.com>
|
CC @wangxiyuan |
|
The CI has passed @wangxiyuan |
What this PR does / why we need it?
Fix #3891.
The empty of
moe_comm_methodin the above issue is due to the wrong check for MoE models. To be specific, the methodis_moe_modelonly checks whether a text-only model is a MoE model, without considering multi-modal models, e.g.,VLandOmni.Config of text-only models looks like:
We can verify MoE models by checking if there is a key named
num_experts.This is different from
VLorOmnimodels, whose config files look like:We can verify MoE models by checking if there is a key named
num_expertsintext_configdict.Part of #3508.
Does this PR introduce any user-facing change?
How was this patch tested?
Update: 2025/11/03
Check the config dict recursively to find if it has a key contains "expert", without checking the model architecture.
It is worth noting that, we can't verify a model by if it contains
FusedMoEmodule becauseis_moe_modelis called somewhere before the model loading, e.g., it's called when updating the ACLGraph config in platform initialization.