[Test]Add ut test qwen3_moe and sfa #4121
Conversation
Signed-off-by: CodeNine-CJ <[email protected]>
|
👋 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
This pull request adds unit tests for Qwen3_moe and torchair_sfa, which is a valuable contribution to improving test coverage. My review has identified a couple of areas for improvement. The test for CustomSparseMoeBlock is currently very basic and should be expanded to cover the forward method's logic. Additionally, a typo in the test setup for AscendSFATorchairImpl could lead to incorrect test behavior and needs to be corrected. Please see the detailed comments for suggestions.
| def test_init(self, mocker: MockerFixture, setup_csmb): | ||
| custom_moe_block = setup_csmb | ||
| assert isinstance(custom_moe_block, CustomSparseMoeBlock) |
There was a problem hiding this comment.
The test coverage for CustomSparseMoeBlock is insufficient. The current test only verifies that the class can be instantiated, but it does not test the forward method, which contains the core logic. To ensure the correctness of the implementation, please add unit tests for the forward method. These tests should cover different execution paths, such as prefill and decode phases.
| def setUp(self, ascend_config, vllm_config, mock_get_tp_size, mock_tp): | ||
| mock_tp.world_size = 2 | ||
| ascend_config.torchair_graph_config.enabled = True | ||
| ascend_config.torchari_graph_config.enable_kv_nz = False |
There was a problem hiding this comment.
There is a typo in the attribute name. It should be torchair_graph_config instead of torchari_graph_config. This typo causes the enable_kv_nz configuration to not be set correctly on the mock object, potentially leading to incorrect test behavior as the code under test reads from ascend_config.torchair_graph_config.enable_kv_nz.
| ascend_config.torchari_graph_config.enable_kv_nz = False | |
| ascend_config.torchair_graph_config.enable_kv_nz = False |
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
### What this PR does / why we need it? Currently, the UT tests lack coverage for the Qwen3_moe network and torchair_sfa. Therefore, supplementary tests are being added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? by CI - vLLM version: v0.11.0 - vLLM main: vllm-project/vllm@83f478b --------- Signed-off-by: CodeNine-CJ <[email protected]> Signed-off-by: luolun <[email protected]>
### What this PR does / why we need it? Currently, the UT tests lack coverage for the Qwen3_moe network and torchair_sfa. Therefore, supplementary tests are being added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? by CI - vLLM version: v0.11.0 - vLLM main: vllm-project/vllm@83f478b --------- Signed-off-by: CodeNine-CJ <[email protected]> Signed-off-by: hwhaokun <[email protected]>
### What this PR does / why we need it? Currently, the UT tests lack coverage for the Qwen3_moe network and torchair_sfa. Therefore, supplementary tests are being added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? by CI - vLLM version: v0.11.0 - vLLM main: vllm-project/vllm@83f478b --------- Signed-off-by: CodeNine-CJ <[email protected]> Signed-off-by: nsdie <[email protected]>
4 participants
What this PR does / why we need it?
Currently, the UT tests lack coverage for the Qwen3_moe network and torchair_sfa. Therefore, supplementary tests are being added.
Does this PR introduce any user-facing change?
No
How was this patch tested?