add weight transpose check. #2756
Conversation
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
![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
This pull request introduces a check to conditionally transpose weights for MoE layers. My review focuses on improving code correctness and maintainability. I've identified a critical bug that would cause a runtime error due to incorrect attribute access, and also pointed out opportunities to remove dead code and reduce duplication for better long-term maintenance.
| w2_data = self._maybe_pad_weight(layer.w2_weight.data).transpose( | ||
| 1, 2).contiguous() | ||
| layer.w2_weight = torch.nn.Parameter(w2_data, requires_grad=False) | ||
| if self.transpose: |
There was a problem hiding this comment.
The self in process_weights_after_loading refers to an instance of UnquantizedFusedMoEMethod, which does not have a transpose attribute. The transpose attribute is defined on the AscendFusedMoE layer instance, which is passed as the layer argument to this method. Accessing self.transpose will raise an AttributeError at runtime. You should access this attribute via the layer object instead.
| if self.transpose: | |
| if layer.transpose: |
vllm_ascend/ops/common_fused_moe.py
Outdated
|
|
||
| original_unquantized_fused_moe_init_func = UnquantizedFusedMoEMethod.__init__ | ||
|
|
||
| is_transpose = True |
There was a problem hiding this comment.
The module-level variable is_transpose is introduced but never used within the file. This appears to be dead code and could cause confusion for future maintenance. If it's not needed, it should be removed.
vllm_ascend/ops/common_fused_moe.py
Outdated
| if expert_data.shape[1] != loaded_weight.shape[1]: | ||
| loaded_weight = loaded_weight.transpose(1,2).contiguous() | ||
| if not is_310p(): | ||
| loaded_weight = torch_npu.npu_format_cast( | ||
| loaded_weight, ACL_FORMAT_FRACTAL_NZ) | ||
| self.transpose = False |
There was a problem hiding this comment.
This block of code for checking and transposing weights is nearly identical to the one in the _load_w13 method (lines 455-460). This duplication can make the code harder to maintain, as any changes would need to be applied in both places. To improve code quality and avoid potential bugs from inconsistent updates, consider extracting this logic into a shared private helper method.
|
👋 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. |
momo609
force-pushed
the
weightloader
branch
from
2c04f20 to
f6ed135
Compare
momo609
force-pushed
the
weightloader
branch
3 times, most recently
from
ab0fca2 to
76a7aa5
Compare
momo609
force-pushed
the
weightloader
branch
5 times, most recently
from
0ba2169 to
ad1aa5a
Compare
Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
momo609
force-pushed
the
weightloader
branch
from
29c69df to
c293c37
Compare
### What this PR does / why we need it? In reinforcement learning scenarios, weight updates are required, but the current inference applies a transpose operation to the weights, altering their shape. This causes a shape mismatch with the training weights, triggering an error during weight updates. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@6fb2788 Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com> Co-authored-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
### What this PR does / why we need it? In reinforcement learning scenarios, weight updates are required, but the current inference applies a transpose operation to the weights, altering their shape. This causes a shape mismatch with the training weights, triggering an error during weight updates. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@6fb2788 Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com> Co-authored-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
### What this PR does / why we need it? In reinforcement learning scenarios, weight updates are required, but the current inference applies a transpose operation to the weights, altering their shape. This causes a shape mismatch with the training weights, triggering an error during weight updates. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@6fb2788 Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com> Co-authored-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
### What this PR does / why we need it? In reinforcement learning scenarios, weight updates are required, but the current inference applies a transpose operation to the weights, altering their shape. This causes a shape mismatch with the training weights, triggering an error during weight updates. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@6fb2788 Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com> Co-authored-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
What this PR does / why we need it?
In reinforcement learning scenarios, weight updates are required, but the current inference applies a transpose operation to the weights, altering their shape. This causes a shape mismatch with the training weights, triggering an error during weight updates.
Does this PR introduce any user-facing change?
How was this patch tested?