[Misc] Make benchmarks use EngineArgs #9529
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update the benchmark scripts to directly use the CLI arguments provided by EngineArgs instead of duplicating a subset of these arguments in each benchmark script. Currently the CLI arguments are duplicated, forcing changes to be made in multiple locations and resulting in some useful vLLM options not being exposed in the scripts. For example, the --num-scheduler-steps option is currently available in benchmark_throughput.py but not benchmark_latency.py, making it difficult to understand the latency impacts of this option. As another example, the benchmark_prioritization.py script appears to be broken currently because it was not updated to expose the --scheduling-policy option which is required for enabling priority. These maintenance challenges are eliminated by using EngineArgs.add_cli_args to add support for all engine arguments directly, and then passing these options to the engine initialization. One minor change in behavior is that when benchmark_throughput.py runs in async mode it no longer includes hard-coded settings for worker_use_ray=False (which is deprecated anyway) and disable_log_requests=True (but the user now has the option to pass --disable-log-requests on the command-line). Similarly, benchmark_prefix_caching no longer has hard-coded values for trust_remote_code=True and enforce_eager=True, but these may now be passed on the command-line.
|
👋 Hi! Thank you for contributing to the vLLM project. 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 do one of these:
🚀 |
comaniac
approved these changes
Oct 19, 2024
| @@ -190,9 +182,7 @@ def main(args): | |||
| default='128:256', | |||
| help='Range of input lengths for sampling prompts,' | |||
| 'specified as "min:max" (e.g., "128:256").') | |||
| parser.add_argument("--seed", | |||
There was a problem hiding this comment.
Do we have "seed" in the engine arg as well?
There was a problem hiding this comment.
|
Also cc @KuntaiDu |
mgoin
approved these changes
Oct 21, 2024
charlifu
pushed a commit
to charlifu/vllm
that referenced
this pull request
Oct 23, 2024
Signed-off-by: charlifu <[email protected]>
Alvant
pushed a commit
to compressa-ai/vllm
that referenced
this pull request
Oct 26, 2024
Signed-off-by: Alvant <[email protected]>
MErkinSag
pushed a commit
to MErkinSag/vllm
that referenced
this pull request
Oct 26, 2024
Signed-off-by: Erkin Sagiroglu <[email protected]>
garg-amit
pushed a commit
to garg-amit/vllm
that referenced
this pull request
Oct 28, 2024
Signed-off-by: Amit Garg <[email protected]>
FerdinandZhong
pushed a commit
to FerdinandZhong/vllm
that referenced
this pull request
Oct 29, 2024
Signed-off-by: qishuai <[email protected]>
NickLucche
pushed a commit
to NickLucche/vllm
that referenced
this pull request
Oct 31, 2024
Signed-off-by: NickLucche <[email protected]>
NickLucche
pushed a commit
to NickLucche/vllm
that referenced
this pull request
Oct 31, 2024
Signed-off-by: NickLucche <[email protected]>
sumitd2
pushed a commit
to sumitd2/vllm
that referenced
this pull request
Nov 14, 2024
Signed-off-by: Sumit Dubey <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update the benchmark scripts to directly use the CLI arguments provided by EngineArgs instead of duplicating a subset of these arguments in each benchmark script.
Currently the CLI arguments are duplicated, forcing changes to be made in multiple locations and resulting in some useful vLLM options not being exposed in the scripts. For example, the --num-scheduler-steps option is currently available in benchmark_throughput.py but not benchmark_latency.py, making it difficult to understand the latency impacts of this option. As another example, the benchmark_prioritization.py script appears to be broken currently because it was not updated to expose the --scheduling-policy option which is required for enabling priority.
These maintenance challenges are eliminated by using EngineArgs.add_cli_args to add support for all engine arguments directly, and then passing these options to the engine initialization.
One minor change in behavior is that when benchmark_throughput.py runs in async mode it no longer includes hard-coded settings for worker_use_ray=False (which is deprecated anyway) and disable_log_requests=True (but the user now has the option to pass --disable-log-requests on the command-line).
Similarly, benchmark_prefix_caching no longer has hard-coded values for trust_remote_code=True and enforce_eager=True, but these may now be passed on the command-line.