Skip to content

Conversation

@ForBetterCodeNine
Copy link
Contributor

@ForBetterCodeNine ForBetterCodeNine commented Nov 20, 2025

What this PR does / why we need it?

The current community lacks unit tests (UT) for files such as torchair_worker, mtp_proposer, and model_runner. Therefore, UT coverage for these files needs to be added.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Signed-off-by: CodeNine-CJ <[email protected]>
@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 adds unit tests for several torchair components. While adding tests is a great initiative, the new test files for NPUTorchairModelRunner and TorchairMtpProposer contain several critical syntax and logical errors that will prevent them from running. I've left detailed comments on these issues. Please address them to ensure the new tests are effective.

from vllm.config import VllmConfig


class TestNPUTorchairModelRunner(PytestBase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

PytestBase is not defined, which will cause a NameError at runtime. You need to import it, likely from tests.ut.base.

device = torch.device("npu:0")

ascend_config = MagicMock()
ascend_config = enable_shared_expert_dp = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This line incorrectly reassigns ascend_config to the boolean value False, overwriting the MagicMock object created on the previous line. This will cause an AttributeError on the next line. It should be an attribute assignment.

Suggested change
ascend_config = enable_shared_expert_dp = False
ascend_config.enable_shared_expert_dp = False

sys.modules[__name__].vllm_version_is = vllm_version_is


class TestTorchairMtpProposer(PytestBase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

PytestBase is not defined, which will cause a NameError at runtime. You need to import it, likely from tests.ut.base.

Comment on lines 64 to 73
def test_init(self, setup_torchair_mtp_proposer):
proposer, _, _, = setup_torchair_mtp_proposer

assert isinstance(proposer, setup_torchair_mtp_proposer)
assert proposer.torchair_compiled_model is None
assert proposer.torchair_compiled_models = {}
Mock.assert_called_once_with(
proposer.__class__.__bases__[0],
proposer.vllm_config,
proposer.device,
proposer.runner
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The test_init method has several syntax and logical errors that will prevent it from running correctly:

  1. Line 65: proposer, _, _, = ... is a syntax error due to the trailing comma.
  2. Line 67: isinstance(proposer, setup_torchair_mtp_proposer) is incorrect. The second argument should be the TorchairMtpProposer class, not the fixture function. You'll also need to import TorchairMtpProposer.
  3. Line 69: assert proposer.torchair_compiled_models = {} is an assignment, not a comparison. This is a syntax error.
  4. Lines 70-75: Mock.assert_called_once_with is called on the Mock class, not a mock instance. You should patch the superclass __init__ and assert on the mock object returned by the patch.

)

mock_get_layers = mocker.patch("vllm.config.get_layers_from_vllm_config")
assert mock_get_layers.call_count = 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This line contains an assignment (=) within an assert statement, which is a syntax error. It should be a comparison (==).

        assert mock_get_layers.call_count == 2

@ForBetterCodeNine ForBetterCodeNine force-pushed the worker_ut branch 6 times, most recently from ab75b53 to f6bee66 Compare November 20, 2025 03:29
Signed-off-by: CodeNine-CJ <[email protected]>
@ForBetterCodeNine ForBetterCodeNine force-pushed the worker_ut branch 17 times, most recently from 1c24929 to 0b8b27c Compare November 20, 2025 12:37
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]>
@ForBetterCodeNine ForBetterCodeNine force-pushed the worker_ut branch 11 times, most recently from 7f84226 to ed14a1a Compare November 21, 2025 06:26
Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: CodeNine-CJ <[email protected]>
@wangxiyuan wangxiyuan merged commit e332e27 into vllm-project:main Nov 21, 2025
18 checks passed
845473182 pushed a commit to 845473182/vllm-ascend that referenced this pull request Nov 21, 2025
…cend into eplb_ci_bugfix

* 'eplb_ci_bugfix' of https://github.com/845473182/vllm-ascend: (31 commits)
  [Test] Add ut test for torchair (vllm-project#4287)
  [Feat][Doc] Add a load_balance_dp_proxy in examples and external dp doc. (vllm-project#4265)
  [CI] Defaultly compile vllm with multimodal audio feature in dockerfile (vllm-project#4324)
  [MM][Bugfix] Add error log for VL models when enabling FLASHCOMM (vllm-project#4272)
  [Readme] EPLB Support Scenarios (vllm-project#4314)
  eplb redundant expert bugfix (vllm-project#4291)
  [Feat][BugFix]Support the Qwen3-Next-80B-A3B-Instruct quantization model&Fix the NZ issue (vllm-project#4245)
  [Test] Add ACL graph capture/replay DP test (vllm-project#4259)
  [Test] quick fix mla ut (vllm-project#4318)
  [Feat] Support MTP to running in full graph mode (vllm-project#3892)
  [CI] Add mla ut (vllm-project#4280)
  [Test] Add tests for the multi-node DeepSeek-V2-Lite network in GE Graph  (vllm-project#4039)
  avoid mrope fusion op when running qwen2.5-vl on a+x machine (vllm-project#4270)
  [Bugfix] fix nightly multi-node EPLB tests' "DYNAMIC_EPLB=true" environment not working (vllm-project#4223)
  [long seq feat]GQA support long-prefill-token-threshold and fixbug (vllm-project#4209)
  [misc] clean up get_metadata_cls (vllm-project#4276)
  [Docs] Improve the AISBench multi-modal testing docs (vllm-project#4255)
  [doc]fix readme for kv pool user guide  (vllm-project#4271)
  remove get_metadata_cls (vllm-project#4087)
  [Bugfix] fix hang in async scheduling (vllm-project#4233)
  ...
845473182 pushed a commit to 845473182/vllm-ascend that referenced this pull request Nov 21, 2025
### What this PR does / why we need it?
The current community lacks unit tests (UT) for files such as
torchair_worker, mtp_proposer, and model_runner. Therefore, UT coverage
for these files needs to be added.

### Does this PR introduce _any_ user-facing change?
NO
### How was this patch tested?

- vLLM version: v0.11.0
- vLLM main:
vllm-project/vllm@2918c1b

---------

Signed-off-by: CodeNine-CJ <[email protected]>
Signed-off-by: 白永斌 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants