Skip to content

Conversation

Potabk
Copy link
Collaborator

@Potabk Potabk commented Sep 5, 2025

What this PR does / why we need it?

vLLM now names the process with VLLM prefix after vllm-project/vllm#21445, we should kill the correct process name after one iteration benchmark to avoid OOM issue

Does this PR introduce any user-facing change?

How was this patch tested?

Signed-off-by: wangli <wangli858794774@gmail.com>
Signed-off-by: wangli <wangli858794774@gmail.com>
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 correctly adds a command to terminate vLLM processes using their new VLLM process name, which is an important fix for resource cleanup in the benchmark suite. However, I've identified a critical issue with an existing command in the same function that indiscriminately kills all python3 processes. This is a significant risk and could lead to terminating unrelated processes. My review includes a suggestion to remove this hazardous command, thereby improving the script's safety and reliability.

Comment on lines 80 to +82
pgrep python3 | xargs -r kill -9

# vLLM now names the process with VLLM prefix after https://github.com/vllm-project/vllm/pull/21445
pgrep VLLM | xargs -r kill -9
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

Killing all processes named python3 is overly broad and poses a significant risk of terminating unrelated user or system processes, which could lead to data loss or system instability. This is a critical issue for script robustness.

Since vLLM processes are now specifically identified by the VLLM prefix, the generic pgrep python3 command is likely redundant and dangerous. It should be removed to make the cleanup process more targeted and safe.

Suggested change
pgrep python3 | xargs -r kill -9
# vLLM now names the process with VLLM prefix after https://github.com/vllm-project/vllm/pull/21445
pgrep VLLM | xargs -r kill -9
# vLLM now names the process with VLLM prefix after https://github.com/vllm-project/vllm/pull/21445
pgrep VLLM | xargs -r kill -9

@MengqingCao MengqingCao added performance-test enable performance test for PR ready-for-test start test by label for PR labels Sep 5, 2025
Copy link

github-actions bot commented Sep 5, 2025

👋 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.

Signed-off-by: wangli <wangli858794774@gmail.com>
@Yikun Yikun added ready-for-test start test by label for PR and removed ready-for-test start test by label for PR labels Sep 5, 2025
@wangxiyuan wangxiyuan merged commit 2967e5e into vllm-project:main Sep 7, 2025
16 checks passed
Angazenn pushed a commit to Angazenn/vllm-ascend that referenced this pull request Sep 10, 2025
…m-project#2782)

### What this PR does / why we need it?
vLLM now names the process with VLLM prefix after
vllm-project/vllm#21445, we should kill the
correct process name after one iteration benchmark to avoid OOM issue
### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@e599e2c

---------

Signed-off-by: wangli <wangli858794774@gmail.com>
offline893 pushed a commit to offline893/vllm-ascend that referenced this pull request Sep 16, 2025
…m-project#2782)

### What this PR does / why we need it?
vLLM now names the process with VLLM prefix after
vllm-project/vllm#21445, we should kill the
correct process name after one iteration benchmark to avoid OOM issue
### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@e599e2c

---------

Signed-off-by: wangli <wangli858794774@gmail.com>
Signed-off-by: offline0806 <z00858301@china.huawei.com>
wangxiaoteng888 pushed a commit to LCAIZJ/vllm-ascend that referenced this pull request Sep 25, 2025
…m-project#2782)

### What this PR does / why we need it?
vLLM now names the process with VLLM prefix after
vllm-project/vllm#21445, we should kill the
correct process name after one iteration benchmark to avoid OOM issue
### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@e599e2c

---------

Signed-off-by: wangli <wangli858794774@gmail.com>
chopper0126 pushed a commit to chopper0126/vllm-ascend that referenced this pull request Sep 26, 2025
…m-project#2782)

### What this PR does / why we need it?
vLLM now names the process with VLLM prefix after
vllm-project/vllm#21445, we should kill the
correct process name after one iteration benchmark to avoid OOM issue
### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@e599e2c

---------

Signed-off-by: wangli <wangli858794774@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance-test enable performance test for PR ready-for-test start test by label for PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants