Skip to content

Add unit tests for batched guided and non-guided requests#23389

Merged
simon-mo merged 1 commit intovllm-project:mainfrom
sarckk:guided-decode-tests
Aug 22, 2025
Merged

Add unit tests for batched guided and non-guided requests#23389
simon-mo merged 1 commit intovllm-project:mainfrom
sarckk:guided-decode-tests

Conversation

@sarckk
Copy link
Collaborator

@sarckk sarckk commented Aug 22, 2025

Purpose

Context: A bug was introduced in #21862 that causes non-guided requests to output incorrect values when batched together with guided requests. See #22896 for technical details of this bug. This bug is fixed by either #22896 or #22963.

This PR adds a unit test that would've caught this bug.

Test Plan

pytest tests/v1/entrypoints/llm/test_struct_output_generate.py::test_structured_output_batched_with_non_guided_requests

Test Result

Trunk, test passes:

Prompt:
"Give an example JSON for an employee profile that fits this schema. Make the response as short as possible. Schema: {'type': 'object', 'properties': {'name': {'type': 'string'}, 'age': {'type': 'integer'}, 'skills': {'type': 'array', 'items': {'type': 'string'}}, 'grade': {'type': 'string', 'pattern': '^[A-D]$'}, 'email': {'type': 'string', 'pattern': '^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\\\.[a-zA-Z]{2,}$'}, 'work_history': {'type': 'array', 'items': {'type': 'object', 'properties': {'company': {'type': 'string'}, 'duration': {'type': 'number', 'minimum': 0.0, 'maximum': 100.0}, 'position': {'type': 'string'}}, 'required': ['company', 'duration', 'position'], 'additionalProperties': False}, 'minItems': 0, 'maxItems': 3}}, 'required': ['name', 'age', 'skills', 'grade', 'email', 'work_history'], 'additionalProperties': False}"
Generated text:
'{"name": "John", "age": 30, "skills": ["python", "javascript", "react"], "grade": "A", "email": "john@example.com", "work_history": [{"company": "ABC", "duration": 5, "position": "Software Engineer"}, {"company": "XYZ", "duration": 3, "position": "DevOps Engineer"}, {"company": "PQR", "duration": 2, "position": "SDE-1"}]}'
Prompt:
'The diameter of the Earth in kilometers is '
Generated text:
'12,742. The radius of the Earth is half of the diameter. What'

After undoing fixes in both #22896 and #22963 (outlines backend seems fine).

FAILED tests/v1/entrypoints/llm/test_struct_output_generate.py::test_structured_output_batched_with_non_guided_requests[guidance] - AssertionError: assert '12,742' in '12!!!!!!!!!!!!!!!'
FAILED tests/v1/entrypoints/llm/test_struct_output_generate.py::test_structured_output_batched_with_non_guided_requests[xgrammar] - AssertionError: assert '12,742' in '12!!!!!!!!!!!!!!!'

(Optional) Documentation Update


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.

Signed-off-by: Yong Hoon Shin <yhshin@meta.com>
@mergify mergify bot added the v1 label Aug 22, 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 adds a valuable unit test to catch a bug related to batching guided and non-guided requests. The test case is well-structured and directly addresses the scenario described. My main feedback is a critical improvement to make the test more robust by ensuring proper resource cleanup, which will prevent potential test flakiness in the future.

@sarckk
Copy link
Collaborator Author

sarckk commented Aug 22, 2025

@mgoin @russellb @JartX follow up to #22896

@mgoin mgoin added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 22, 2025
Copy link
Member

@mgoin mgoin left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, let's see if CI green. Thanks!

@sarckk
Copy link
Collaborator Author

sarckk commented Aug 22, 2025

@mgoin the newly added test passed in ci/pr/v1-test. A single unrelated test failed with error:

huggingface_hub.errors.HfHubHTTPError: 504 Server Error: Gateway Timeout for url: https://huggingface.co/api/models/google/gemma-2-2b-it/tree/main?recursive=False&expand=False (Request ID: Root=1-68a7c260-6f4f44cc3173e43632f8da4e;8e4182ef-b4a0-4bbd-87a2-2457f179fb11)

@simon-mo simon-mo merged commit b6d7d34 into vllm-project:main Aug 22, 2025
25 of 27 checks passed
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 28, 2025
xiao-llm pushed a commit to xiao-llm/vllm that referenced this pull request Aug 28, 2025
…ct#23389)

Signed-off-by: Yong Hoon Shin <yhshin@meta.com>
Signed-off-by: Xiao Yu <xiao.yu@amd.com>
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Aug 28, 2025
mengxingkongzhouhan pushed a commit to mengxingkongzhouhan/vllm that referenced this pull request Aug 30, 2025
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Sep 3, 2025
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
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 v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants