[CI/Build][Doc] Move existing benchmark scripts in CI/document/example to vllm bench CLI #21355
Conversation
mergify
bot
added
documentation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
![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 refactors the benchmark invocation by replacing direct script calls (python benchmarks/benchmark_*.py) with the new vllm bench command-line interface. The changes are applied consistently across CI scripts, documentation, and examples. Additionally, the old benchmark scripts are now marked as deprecated, which is a good practice for a smooth transition. The changes improve maintainability by centralizing the benchmark entry points. I've reviewed the changes and found no high or critical issues.
|
This pull request has merge conflicts that must be resolved before it can be |
yeqcharlotte
requested review from
DarkLight1337,
LucasWilkinson,
ProExpertProg,
WoosukKwon,
aarnphm,
alexm-redhat,
comaniac,
jeejeelee,
mgoin,
njhill,
robertgshaw2-redhat,
tlrmchlsmth,
youkaichao and
zou3519
as code owners
There was a problem hiding this comment.
I would even recommend just nuke this file, or some sort of tombstone, maybe too aggressive 😓
There was a problem hiding this comment.
we can do that as a next step. wait for 1 weekto verify i don't break CI/CD in any weird ways lol.
There was a problem hiding this comment.
Yeah, one more PR is good.
github-actions
bot
added
the
ready
benchmarks/auto_tune/auto_tune.sh
Outdated
| prefix_len=$(( INPUT_LEN * MIN_CACHE_HIT_PCT / 100 )) | ||
| adjusted_input_len=$(( INPUT_LEN - prefix_len )) | ||
| python3 benchmarks/benchmark_serving.py \ | ||
| vllm3 bench serve \ |
| @deprecated( | ||
| "benchmark_serving.py is deprecated and will be removed in a future " | ||
| "version. Please use 'vllm bench serve' instead.", | ||
| ) |
There was a problem hiding this comment.
@yeqcharlotte you are going to break all of our benchmark scripts 😆
I didn't realize that vllm serve used different code from benchmark_serving.py -- One downside of this change is that it will require installing vllm in order to run benchmarks IIUC.
There was a problem hiding this comment.
How did you use it? This should just throw warning at the beginning instead of crashing everything instantly.
For most single host benchmark, before running benchmark_serving.py you would probably have already run vllm serve that requires install vllm anyway?
If you are benchmarking against remote, then … this client script itself is probably not good enough anyway 😅
Potentially we can split the packaging later to allow bench script to be installed separately pip install vllm[bench] that won’t install all dependency if vllm serve?
There was a problem hiding this comment.
I'm personally okay with this change: right now it's a bit weird that we have two versions of benchmark code and I do want to get rid of one as soon as possible to avoid confusion and maintenance overhead, and I think Charlotte made some good points on why we should move them into vllm.
There was a problem hiding this comment.
it's very difficult to guard the standalone scripts to prevent it from importing vllm. also, i was able to install and run the bench commands on CPU node as well.
There was a problem hiding this comment.
@tlrmchlsmth given the discussion, are you ok if we merge this?
There was a problem hiding this comment.
it's very difficult to guard the standalone scripts to prevent it from importing vllm. also, i was able to install and run the bench commands on CPU node as well.
This is true. Even though benchmark_serving.py tries to support this case it's broken 😆
If you are benchmarking against remote, then … this client script itself is probably not good enough anyway 😅
@yeqcharlotte the usecase I'm thinking of is in-cluster benchmarking from a container that doesn't have the CUDA runtime or GPUs. Do you see any problems with using it in that case?
Signed-off-by: Ye (Charlotte) Qi <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]> Signed-off-by: x22x22 <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]> Signed-off-by: Paul Pak <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]>
…e to vllm bench CLI (vllm-project#21355) Signed-off-by: Ye (Charlotte) Qi <[email protected]>
7 participants
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
We are planning for a few more changes in benchmarking script. Before doing that, move existing reference in documentation, CI and examples on benchmark_.*.py so we don't have to add them in 2 places. FIX #21206.
Test Plan
Rely on CI for the rest.
Test Result
It ran
(Optional) Documentation Update