Skip to content

Conversation

@yeqcharlotte
Copy link
Collaborator

@yeqcharlotte yeqcharlotte commented Sep 8, 2025

Purpose

Follow up of #21355. Delete old benchmarks/benchmark_(latency|throughput|serving).py to avoid confusion for contributing into these scripts.

Given CI has been running on the new script for 1 month so far, it should be fine to delete these scripts.

Test Plan

Make sure no more references in: https://github.com/search?q=repo%3Avllm-project%2Fvllm+%2Fbenchmark_%28latency%7Cserving%7Cthroughput%29.py%2F&type=code

Test current output:

python benchmarks/benchmark_latency.py

Test Result

terminal shows it prints deprecation and exits 1
image


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@yeqcharlotte yeqcharlotte requested review from ywang96 and removed request for hmellor September 8, 2025 04:39
@mergify mergify bot added documentation Improvements or additions to documentation performance Performance-related issues labels Sep 8, 2025
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 effectively deprecates the old benchmark scripts (benchmark_latency.py, benchmark_serving.py, benchmark_throughput.py) by replacing their contents with a helpful message pointing to the new vLLM CLI commands. The documentation in benchmarks/README.md is also updated accordingly. My main feedback is to improve the deprecation scripts to exit with a non-zero status code and print to stderr, which is crucial for automation and CI/CD pipelines to correctly detect that the old scripts are no longer functional.

@yeqcharlotte yeqcharlotte added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 8, 2025
@mergify
Copy link

mergify bot commented Sep 8, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @yeqcharlotte.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 8, 2025
Copy link
Member

@hmellor hmellor Sep 8, 2025

Choose a reason for hiding this comment

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

Maybe something for a follow up PR but could the content of this README be moved to docs/contributing/benchmarks.md? Right now the information about benchmarking in the docs is quite sparse and this README contains loads of useful information

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good idea. possibly i can follow up with @ywang96 to see if we can take a more systematic approach about all these random benchmark scripts. :D

@yeqcharlotte yeqcharlotte force-pushed the delete_old_bench branch 2 times, most recently from 7c5cf6e to 77f9f70 Compare September 9, 2025 08:10
Signed-off-by: Ye (Charlotte) Qi <[email protected]>
Signed-off-by: Ye (Charlotte) Qi <[email protected]>
Signed-off-by: Ye (Charlotte) Qi <[email protected]>
Signed-off-by: Ye (Charlotte) Qi <[email protected]>
Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

LGTM!

@hmellor hmellor enabled auto-merge (squash) September 9, 2025 08:15
@hmellor hmellor merged commit 6fb2788 into vllm-project:main Sep 9, 2025
17 checks passed
eicherseiji pushed a commit to eicherseiji/vllm that referenced this pull request Sep 9, 2025
skyloevil pushed a commit to skyloevil/vllm that referenced this pull request Sep 13, 2025
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
…ghput / latency (vllm-project#24411)

Signed-off-by: Ye (Charlotte) Qi <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
…ghput / latency (vllm-project#24411)

Signed-off-by: Ye (Charlotte) Qi <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation performance Performance-related issues ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants