Skip to content

Conversation

@ForBetterCodeNine
Copy link
Contributor

@ForBetterCodeNine ForBetterCodeNine commented Oct 27, 2025

What this PR does / why we need it?

There is currently a lack of test cases for TorchAir in the GE graph mode. We have supplemented a test case for this scenario, using a lightweight network (Qwen3-30B) for testing.

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 end-to-end tests for TorchAir in GE graph mode. The changes introduce a new test file with several test cases. However, there is a critical issue with a redefined test fixture function, which will cause tests to fail. Additionally, one of the test fixtures is missing an assertion to validate the output. These issues need to be addressed to ensure the tests are functional and effective.

Comment on lines +170 to +174
def _qwen_torchair_test_fixture(
model,
tp,
enable_expert_parallel,
):
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 function _qwen_torchair_test_fixture is defined twice in this file. The first definition is on line 33, and this second definition on line 170 overwrites it. This will cause a TypeError for tests that are intended to call the first version of the function (e.g., test_e2e_qwen_with_torchair) due to mismatched arguments. Please rename this function and its call sites (test_e2e_qwen2_with_torchair and test_e2e_qwen3_moe_with_torchair) to resolve the name collision.

Comment on lines +222 to +223
for i in range(len(vllm_output)):
print(f"Generated text: {vllm_output[i][1]!r}")
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This test fixture is missing an assertion to verify the generated output against the golden_results. The loop currently only prints the generated text. Please add an assertion to ensure the test correctly validates the model's output.

Suggested change
for i in range(len(vllm_output)):
print(f"Generated text: {vllm_output[i][1]!r}")
for i in range(len(vllm_output)):
assert golden_results[i] == vllm_output[i][1]
print(f"Generated text: {vllm_output[i][1]!r}")

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.

1 participant