Skip to content

Conversation

@Chenyaaang
Copy link
Contributor

@Chenyaaang Chenyaaang commented Apr 1, 2025

In benchmark_serving_structured_output.py, when dataset is json, use the original loaded schema instead of creating json_schemas.

The sample command provided in benchmark_serving_structured_output.py doesn't work because it fails at sample_requests() for json dataset.
python benchmarks/benchmark_serving_structured_output.py
--backend
--model <your_model>
--dataset json
--structured-output-ratio 1.0
--structured-output-backend xgrammar
--request-rate 10
--num-prompts 1000

Signed-off-by: Chenyaaang <chenyangli@google.com>
@github-actions
Copy link

github-actions bot commented Apr 1, 2025

👋 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.

🚀

@yaochengji yaochengji added the ready ONLY add when PR is ready to merge/full CI is needed label Apr 2, 2025
@yaochengji
Copy link
Collaborator

To better understand the fix, could you please provide a test case that demonstrates the bug you've addressed?

@DarkLight1337
Copy link
Member

I think it would be cleaner to just add another if-else branch.

elif args.dataset == "json":
    json_schemas = [schema]

That way we don't have to modify gen_prompt and get_schema at all

@Chenyaaang
Copy link
Contributor Author

json_schemas

len(json_schemas) should be same as prompt size, so it means we need to copy schema num_prompts times (that's unncessary)

@Chenyaaang
Copy link
Contributor Author

To better understand the fix, could you please provide a test case that demonstrates the bug you've addressed?

Added in the description.

@DarkLight1337
Copy link
Member

DarkLight1337 commented Apr 3, 2025

json_schemas

len(json_schemas) should be same as prompt size, so it means we need to copy schema num_prompts times (that's unncessary)

How about json_schemas = [schema] * args.num_prompts?

Edit: actually it doesn't look like they need to be of the same length since we apply modulus operator before indexing the list of schemas.

@mergify mergify bot added the tpu Related to Google TPUs label Apr 9, 2025
Signed-off-by: Chenyaaang <chenyangli@google.com>
@mergify mergify bot removed the tpu Related to Google TPUs label Apr 10, 2025
@Chenyaaang
Copy link
Contributor Author

json_schemas

len(json_schemas) should be same as prompt size, so it means we need to copy schema num_prompts times (that's unncessary)

How about json_schemas = [schema] * args.num_prompts?

Edit: actually it doesn't look like they need to be of the same length since we apply modulus operator before indexing the list of schemas.

Thanks, fixed. PTAL.

Copy link
Member

@DarkLight1337 DarkLight1337 left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with me!

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) April 10, 2025 17:48
@DarkLight1337 DarkLight1337 merged commit 5fbab20 into vllm-project:main Apr 10, 2025
25 checks passed
p88h pushed a commit to p88h/vllm that referenced this pull request Apr 10, 2025
Signed-off-by: Chenyaaang <chenyangli@google.com>
yangw-dev pushed a commit to yangw-dev/vllm that referenced this pull request Apr 21, 2025
Signed-off-by: Chenyaaang <chenyangli@google.com>
Signed-off-by: Yang Wang <elainewy@meta.com>
jikunshang pushed a commit to jikunshang/vllm that referenced this pull request Apr 29, 2025
Signed-off-by: Chenyaaang <chenyangli@google.com>
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025
Signed-off-by: Chenyaaang <chenyangli@google.com>
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
Signed-off-by: Chenyaaang <chenyangli@google.com>
Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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