Skip to content
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

benchmark serving: random + sharegpt dataset #14026

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seungrokj
Copy link

@seungrokj seungrokj commented Feb 28, 2025

Adding random inputs from sharegpt dataset in the "--dataset-name random" from benchmark_serving.py

Background: when comparing online serving performance against sglang. We witnessed the random inputs are different from vllm client vs sglang client and this gives some perf gap in certain models. To narrow this gap, adding new feature like this:

image

Usage:

random inputs (default)

python3 vllm/benchmarks/benchmark_serving.py
--backend vllm
--dataset-name random \

random inputs from sharegpt dataset (added feature)

python3 vllm/benchmarks/benchmark_serving.py
--backend vllm
--dataset-name random
--dataset-path ShareGPT_V3_unfiltered_cleaned_split.json \

Signed-off-by: seungrokj <[email protected]>
Copy link

👋 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 fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 ready label to the PR or enable auto-merge.

🚀

@seungrokj
Copy link
Author

reopened here again:
@andylolu2 @billishyahao @haic0

@DarkLight1337 DarkLight1337 requested review from WoosukKwon, ywang96 and comaniac and removed request for WoosukKwon February 28, 2025 16:43
@comaniac
Copy link
Collaborator

Thanks for the PR. While the feature makes sense, I feel the updated CLI is a bit confusing. Instead of adding the feature of "sampling from ShareGPT" to "random", why don't we enhance the existing sharegpt mode? Intuitively in this case we are still benchmarking ShareGPT, after all.

@ywang96
Copy link
Member

ywang96 commented Mar 1, 2025

We witnessed the random inputs are different from vllm client vs sglang client and this gives some perf gap in certain models.

Hello @seungrokj! Could you clarify a bit more on this? You can use the same client code to benchmark against both vLLM and SGLang, so at least on the input side there shouldn't be any difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants