add new test case for aclgraph capture and replay v0.11.0 #3887
Conversation
Signed-off-by: lilinsiman <[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 a new end-to-end test for aclgraph capture and replay with a data parallelism size of 2. The test correctly uses multiprocessing to simulate a multi-rank environment and patches torch.npu.NPUGraph methods to verify the number of captures and replays. However, I've identified a critical issue with a long sleep at the end of the test, which will severely impact test suite performance, and a high-severity issue regarding the use of magic numbers in assertions, which harms maintainability. Please address these points to improve the test's quality and reliability.
| f"Replay count mismatch. Expected: {expected_total_replay}, Got: {actual_replay}" | ||
| ) | ||
| os.environ["VLLM_WORKER_MULTIPROC_METHOD"] = 'spawn' | ||
| sleep(600) |
There was a problem hiding this comment.
A 10-minute sleep (sleep(600)) at the end of a test is a major concern. It will dramatically increase the execution time of the test suite. This is often a workaround for issues with resource cleanup that are not being handled correctly. Please investigate the root cause that necessitates this long delay and implement a proper fix. If a delay is truly unavoidable, it must be justified with a detailed comment, and a more robust synchronization mechanism should be preferred over a fixed sleep.
| sleep(600) | |
| # FIXME: This long sleep is a workaround and should be removed after fixing the underlying resource cleanup issue. |
| max_num_batch_sizes = math.floor( | ||
| (1800 - num_comm_groups * 40) / num_acl_graphs / | ||
| (1 + num_comm_groups * 2)) |
There was a problem hiding this comment.
The calculation for max_num_batch_sizes uses several magic numbers (1800, 40, 2), which makes the test difficult to understand and maintain. If the underlying logic for this calculation changes in the future, this test will fail without a clear indication of the root cause. Please replace these magic numbers with named constants. This will greatly improve the readability and maintainability of the test. For example:
# Constants from Ascend backend for max_num_batch_sizes calculation.
# Please verify and document their exact meaning.
ACL_GRAPH_RESOURCE_LIMIT = 1800
RESOURCE_COST_PER_COMM_GROUP = 40
REPLAY_OVERHEAD_FACTOR = 2
max_num_batch_sizes = math.floor(
(ACL_GRAPH_RESOURCE_LIMIT - num_comm_groups * RESOURCE_COST_PER_COMM_GROUP) /
num_acl_graphs / (1 + num_comm_groups * REPLAY_OVERHEAD_FACTOR))1 participant
What this PR does / why we need it?
add new test case for aclgraph capture and replay v0.11.0
Does this PR introduce any user-facing change?
no
How was this patch tested?
ut