[310p]: add rmsnorm gated fallback and unit test - #7424
Conversation
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 PR introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback implementation for RMSNormGated for the 310P device, ensuring compatibility by using the native PyTorch implementation instead of a Triton-based one. Unit tests are included to verify the fallback mechanism. The changes appear correct and well-tested.
My main feedback is to update the pull request title and description to align with the repository's style guide. The style guide requires a specific format for the PR title ([Branch][Module][Action] Title) and a structured summary. The current PR title and empty description do not follow this format. Adhering to this helps maintain consistency across the project.
Here are the suggestions based on the repository's style guide:
Suggested PR Title:
[310p][Ops][Feature] Add RMSNormGated fallback and unit testSuggested PR Summary:
### What this PR does / why we need it?
This PR adds a fallback implementation for `RMSNormGated` for the Ascend 310P device. The 310P environment may not support the Triton-based implementation of gated RMS norm. This change introduces `AscendRMSNormGated310`, which inherits from `vllm.model_executor.layers.layernorm.RMSNormGated` and overrides the `forward_oot` method to call the native PyTorch implementation (`forward_native`). This ensures functionality on 310P without relying on Triton kernels.
Unit tests are added to verify that the fallback correctly calls the native implementation, both with and without a gate tensor.
The new `AscendRMSNormGated310` operator is registered for the 310P device.
### Does this PR introduce _any_ user-facing change?
No. This is a backend change to support a specific hardware platform and does not alter any user-facing APIs or behaviors.
### How was this patch tested?
New unit tests have been added in `tests/ut/_310p/ops/test_layernorm_310.py` to verify the behavior of the new `AscendRMSNormGated310` implementation. The tests use mocking to confirm that the `forward_oot` method correctly delegates to the `forward_native` method of the parent class. CI should pass with these new tests.|
👋 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. |
|
您好,想问下您是不是把您最近提交的几个pr合并了 就可以在310p3上跑qwen3.5了? |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
c7bd653 to
9e3e500
Compare
恐怕还得看看,不过我们会尽快的! |
|
### What this PR does / why we need it? RFC vllm-project#7394 310P cannot use the fused `rmsnormgated` operator and must fall back to the native implementation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? ut - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4497431 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
### What this PR does / why we need it? RFC vllm-project#7394 310P cannot use the fused `rmsnormgated` operator and must fall back to the native implementation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? ut - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4497431 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
### What this PR does / why we need it? RFC vllm-project#7394 310P cannot use the fused `rmsnormgated` operator and must fall back to the native implementation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? ut - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4497431 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
### What this PR does / why we need it? RFC vllm-project#7394 310P cannot use the fused `rmsnormgated` operator and must fall back to the native implementation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? ut - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4497431 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
### What this PR does / why we need it? RFC vllm-project#7394 310P cannot use the fused `rmsnormgated` operator and must fall back to the native implementation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? ut - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4497431 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com> Signed-off-by: nanxing <1014662416@qq.com>
### What this PR does / why we need it? RFC vllm-project#7394 310P cannot use the fused `rmsnormgated` operator and must fall back to the native implementation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? ut - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4497431 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
### What this PR does / why we need it? RFC vllm-project#7394 310P cannot use the fused `rmsnormgated` operator and must fall back to the native implementation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? ut - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4497431 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
What this PR does / why we need it?
RFC #7394
310P cannot use the fused
rmsnormgatedoperator and must fall back to the native implementation.Does this PR introduce any user-facing change?
NO
How was this patch tested?
ut