Repository navigation
Conversation
Signed-off-by: main2main-bot <main2main-bot@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adapts the vllm-ascend codebase to significant architectural changes in the upstream vLLM repository. The primary focus is the refactoring of the FusedMoE layer into the new RoutedExperts and MoERunner components. To maintain support for Ascend-specific features, the PR implements targeted monkey-patches for the MoERunner and the FusedMoE factory, ensuring that existing Ascend optimizations are preserved while remaining compatible with the updated upstream API. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
👋 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][Misc] Adapt Ascend FusedMoE to upstream RoutedExperts refactoringSuggested PR Summary:
### What this PR does / why we need it?
This PR adapts the Ascend FusedMoE implementation to the upstream refactoring that introduced `RoutedExperts` and `MoERunner`. It updates `AscendFusedMoE` and `AscendFusedMoE310` to inherit from `RoutedExperts` and monkey-patches the `FusedMoE` factory and `MoERunner` to ensure compatibility.
Feedback has been provided to address potential issues:
- Initialize `self.vllm_config` in `AscendFusedMoE310` to prevent `AttributeError` on 310P.
- Add the `forward` method override to `AscendFusedMoE310` to support legacy callers.
- Filter out duplicate parameters in `_patched_named_parameters` to avoid yielding duplicates.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
CI testing.| # Replace quant_method with Ascend 310P version | ||
| assert self.quant_method is not None | ||
| # Keep base_quant_method aligned with the Ascend-replaced quant_method | ||
| # so FusedMoE.maybe_init_modular_kernel doesn't dispatch into the | ||
| # upstream UnquantizedFusedMoEMethod.maybe_make_prepare_finalize. | ||
| self.base_quant_method = self.quant_method |
There was a problem hiding this comment.
self.vllm_config is not initialized in AscendFusedMoE310.init (unlike AscendFusedMoE which retrieves it via get_current_vllm_config()). Since RoutedExperts.init does not set self.vllm_config, any downstream access to self.vllm_config on 310P will raise an AttributeError.\n\nWe should initialize self.vllm_config using get_current_vllm_config().
| # Replace quant_method with Ascend 310P version | |
| assert self.quant_method is not None | |
| # Keep base_quant_method aligned with the Ascend-replaced quant_method | |
| # so FusedMoE.maybe_init_modular_kernel doesn't dispatch into the | |
| # upstream UnquantizedFusedMoEMethod.maybe_make_prepare_finalize. | |
| self.base_quant_method = self.quant_method | |
| # Replace quant_method with Ascend 310P version\n assert self.quant_method is not None\n self.base_quant_method = self.quant_method\n\n from vllm.config import get_current_vllm_config\n self.vllm_config = get_current_vllm_config() |
| @property | ||
| def is_internal_router(self) -> bool: | ||
| # 310P Ascend path expects router logits from the model forward path. | ||
| return False |
There was a problem hiding this comment.
Unlike AscendFusedMoE, AscendFusedMoE310 is missing the forward method override. Legacy callers that still call forward() directly on the routed experts layer instead of the runner on 310P will bypass the custom _runner / forward_impl path or fail.\n\nWe should add the forward method override to AscendFusedMoE310 to match AscendFusedMoE.
@property\n def is_internal_router(self) -> bool:\n # 310P Ascend path expects router logits from the model forward path.\n return False\n\n def forward(\n self,\n hidden_states: torch.Tensor,\n router_logits: torch.Tensor,\n ) -> torch.Tensor | tuple[torch.Tensor, torch.Tensor]:\n if hasattr(self, \"_runner\"):\n return self._runner.forward(hidden_states, router_logits)\n self._ensure_moe_quant_config_init()\n return self.forward_impl(hidden_states, router_logits)| def _patched_named_parameters(self, prefix="", recurse=True, remove_duplicate=True): | ||
| if recurse and "_modules" in self.__dict__ and "routed_experts" in self._modules: | ||
| routed_experts = self._modules["routed_experts"] | ||
| for name, param in routed_experts.named_parameters(): | ||
| yield prefix + name, param | ||
| yield from _original_named_params( | ||
| self, prefix=prefix, recurse=recurse, remove_duplicate=remove_duplicate | ||
| ) | ||
|
|
||
| moe_runner_mod.MoERunner.named_parameters = _patched_named_parameters |
There was a problem hiding this comment.
The current implementation of _patched_named_parameters yields parameters from routed_experts first, and then calls _original_named_params which will yield the same parameters again with the routed_experts. prefix. Because the two generator calls do not share a parameter memoization set, duplicate parameter tensors will be yielded. This can cause issues (such as ValueError in PyTorch optimizers or duplicate counting in parameter logging/profiling).\n\nWe should filter out the duplicate routed_experts. parameters from the original generator's output.
def _patched_named_parameters(self, prefix=\"\", recurse=True, remove_duplicate=True):\n if recurse and \"_modules\" in self.__dict__ and \"routed_experts\" in self._modules:\n routed_experts = self._modules[\"routed_experts\"]\n for name, param in routed_experts.named_parameters():\n yield prefix + name, param\n for name, param in _original_named_params(\n self, prefix=prefix, recurse=recurse, remove_duplicate=remove_duplicate\n ):\n if recurse and \"routed_experts.\" in name:\n continue\n yield name, param
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| def _forward_impl( | ||
| self, | ||
| layer: torch.nn.Module, | ||
| hidden_states: torch.Tensor, |
There was a problem hiding this comment.
FusedMoE -> RoutedExperts 重构正确性确认
将 FusedMoE 替换为 RoutedExperts 是跟随上游 vLLM API 变迁的必要适配。代码变更量大，需要关注以下几点：
-
AscendFusedMoE 和 AscendFusedMoE310 的
__init__签名：从*args, **kwargs改为显式参数列表，这是好的改进（提高可读性）。但新增了大量 Ascend-specific kwargs（如tid2eid,gate,shared_experts等），这些是通过routed_experts_args传入的。请确认上游RoutedExperts.__init__是否会正确忽略这些 Ascend-specific kwargs，避免unexpected keyword argument错误。 -
_get_quant_method方法：AscendFusedMoE新增了_get_quant_method方法覆盖父类的 quant method 初始化逻辑。请确认这与上游RoutedExperts的_ensure_moe_quant_config_init机制是否兼容，避免 quant_method 被重复初始化或覆盖。
|
Superseded by #12420. |
3 participants
Cumulative Adaptation Summary
Step step-1
Verdict: No-op (no vllm-ascend changes required)
Upstream changes:
vllm/model_executor/models/gemma4_mm.py— Model-specificget_served_model_nameusagevllm/v1/sample/ops/topk_topp_triton.py— XPU block-size tuning (internal)vllm/v1/worker/cpu_worker.py— CPU worker library check improvementsWhy no adaptation:
apply_top_k_top_p_triton; importsTopKTopPSamplerwrapper onlyStep step-2
Verdict: No-op (no vllm-ascend changes required)
Upstream changes:
vllm/config/kv_transfer.py— Docstring-only change removing P2pNcclConnector referencevllm/distributed/kv_transfer/kv_connector/factory.py— RemovedP2pNcclConnectorregistrationvllm/distributed/kv_transfer/kv_connector/v1/moriio/moriio_common.py— Comment-only changevllm/distributed/kv_transfer/kv_connector/v1/p2p/— Entire p2p directory deleted (P2pNcclConnector, P2pNcclEngine, TensorMemoryPool)Why no adaptation:
P2pNcclConnector,P2pNcclEngine,TensorMemoryPool, or thev1.p2pmodulevllm_ascend/distributed/kv_transfer/__init__.pyare entirely Ascend-native (Mooncake, AscendStore, UCM, LMCacheAscend) and unaffected by the upstream removalStep step-3
Verdict: No-op (no vllm-ascend changes required)
Upstream changes:
vllm/_custom_ops.py— Internal refactor: cacherelease_dnnl_matmul_handlerinself.dtorvllm/benchmarks/serve.py— New_align_prompts_to_server_tokenizer()prompt alignment functionvllm/config/quantization.py— AddkFp8StaticChannelSymimport,"fp8_per_channel_static"key,"fp8_per_channel"online shorthandvllm/model_executor/layers/quantization/__init__.py— Add"fp8_per_channel"toQuantizationMethodsLiteralvllm/model_executor/layers/quantization/online/base.py— Import/registerFp8PtpcOnlineLinearMethodandFp8PtpcOnlineMoEMethodvllm/model_executor/layers/quantization/online/fp8.py— NewFp8PtpcOnlineLinearMethodandFp8PtpcOnlineMoEMethodclasses; extend_Fp8OnlineMoEBase.__init__with optional params (weight_key,activation_key,allow_vllm_cutlass, all with defaults preserving previous behavior); addper_act_token_quant/per_out_ch_quantclass attrsWhy no adaptation:
vllm-ascend does not import from
vllm.model_executor.layers.quantization.online(base or fp8) — the online dispatch system is entirely separate from Ascend's quantization configsvllm-ascend does not import from
vllm/config/quantization.py— usesregister_quantization_configfrom thelayers.quantizationpackage insteadvllm-ascend does not subclass
_Fp8OnlineMoEBaseor any online quantization method — Ascend has its ownAscendLinearMethod/AscendFusedMoEMethodwrappersvllm-ascend has zero references to
kFp8StaticChannelSym,_ONLINE_LINEAR_METHODS,_ONLINE_MOE_METHODS,_ONLINE_SHORTHANDS, orCPUDNNLGEMMHandlerThe
_Fp8OnlineMoEBase.__init__signature changes are backward-compatible (all new params have defaults matching previous behavior)All other changes are purely additive (new classes, new dict/Literal entries)
vLLM version: v0.22.1
vLLM main: vllm-project/vllm@967c5c3