-
Notifications
You must be signed in to change notification settings - Fork 466
[Benchmark] Correctly kill vllm process in performance benchamrk #2782
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: wangli <wangli858794774@gmail.com>
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 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.
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 |
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.
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.
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 |
👋 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. |
Signed-off-by: wangli <wangli858794774@gmail.com>
…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>
…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>
…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>
…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>
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?