-
Notifications
You must be signed in to change notification settings - Fork 655
test ge graph for torchair #3783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
There was a problem hiding this 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.
| def _qwen_torchair_test_fixture( | ||
| model, | ||
| tp, | ||
| enable_expert_parallel, | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| for i in range(len(vllm_output)): | ||
| print(f"Generated text: {vllm_output[i][1]!r}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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}") |
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?