[Bugfix] Fix tool_calls Iterable consumed when debug logging is enabled#34844
[Bugfix] Fix tool_calls Iterable consumed when debug logging is enabled#34844wojciech-wais wants to merge 4 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
The pull request effectively resolves the issue where tool_calls iterators were being prematurely exhausted when debug logging was enabled. By changing the type annotation to list in vLLM's internal message types and adding a Pydantic field_validator to eagerly materialize tool_calls in ChatCompletionRequest, the fix ensures that these calls can be accessed multiple times without data loss. The implementation is correct, follows Pydantic v2 best practices for field normalization, and includes comprehensive regression tests. No high or critical severity issues were identified during the review.
|
cc @dtrifiro |
Head branch was pushed to by a user without write access
8124b2d to
9fac590
Compare
9fac590 to
e665f7a
Compare
The OpenAI Python SDK types tool_calls as Iterable[...] in ChatCompletionAssistantMessageParam. When Pydantic v2 validates from Python objects (not JSON), it wraps Iterable fields in a one-shot ValidatorIterator. Debug logging via model_dump_json() consumed that iterator, causing the Mistral tokenizer to see empty tool_calls and raise "ValueError: Unexpected tool call id ...". Fix: add a model_validator(mode='after') on ChatCompletionRequest that materialises any ValidatorIterator tool_calls back to a list after Pydantic validation completes. Also change vllm's own TypedDicts (CustomChatCompletionMessageParam, ConversationMessage) to use list[...] instead of Iterable[...] for tool_calls. Fixes vllm-project#34792 Signed-off-by: Wojciech Wais <wojciech.wais@gmail.com>
e665f7a to
6f83a13
Compare
|
I've rebased changes and fixed failures in test_tool_calls_serialization.py test. |
|
These pydantic lazy iterators have bit me a few times in the past in other places, so solving this generally would be reasonable. There's already some code in both mistral and gpt-oss model paths that do some of this materialization, but only for those models and perhaps not in all cases. However, some of the added unit tests are still failing. Those will need to be investigated and fixed for this to merge. If you need help root-causing those, let me know and I'm happy to help out. Thanks! |
…ators Generators and iterators passed as tool_calls were consumed during Pydantic union type validation before the mode=after validator could materialise them. Switch to mode=before so we convert iterables to lists before Pydantic touches them. Fixes 3 failing tests: test_tool_calls_from_generator_are_materialised, test_multiple_tool_calls_materialised[1], test_multiple_tool_calls_materialised[3] Signed-off-by: Wojciech Wais <wojciech.wais@gmail.com>
0bcfeb1 to
039068a
Compare
mode=before: converts generators/iterators to lists before Pydantic consumes them during union type validation. mode=after: converts Pydantic ValidatorIterator wrappers back to plain lists. Pydantic re-wraps even already-list tool_calls in a ValidatorIterator when validating against the Iterable[...] type in ChatCompletionAssistantMessageParam. Both stages are needed: before prevents iterator exhaustion, after unwraps Pydantic's own lazy wrapper. Signed-off-by: Wojciech Wais <wojciech.wais@gmail.com>
|
Thanks @bbrowning! I've investigated and fixed the failing unit tests. The initial Fixed tests:
Remaining CI failures: |
Fixes #34792.
When
VLLM_LOGGING_LEVEL=debugis set, tool calling with Mistral models fails withValueError: Unexpected tool call id ....Root cause
The OpenAI Python library types
tool_callsasIterable[...]inChatCompletionAssistantMessageParam. When Pydantic v2 validates aChatCompletionRequestfrom Python objects (not from a raw JSON body, as happens in the Anthropic → OpenAI conversion path inside_convert_anthropic_to_openai_request), it wrapsIterablefields in a one-shot lazy iterator. Debug logging then callschat_req.model_dump_json(), which serialises the model and exhausts the iterator. Subsequent readers — notably the Mistral tokenizer'sapply_chat_template— see an emptytool_callssequence and raise the "Unexpected tool call id" error.Fix
vllm/entrypoints/chat_utils.py— changetool_callsfromIterable[...]tolist[...]in both vLLM-owned TypedDicts (CustomChatCompletionMessageParamandConversationMessage). This corrects the type annotation and prevents Pydantic from creating lazy iterators for these vLLM-specific message types.vllm/entrypoints/openai/chat_completion/protocol.py— add afield_validatoronChatCompletionRequest.messagesthat eagerly materialises any non-listtool_callstolist. This covers the external OpenAI library types (e.g.ChatCompletionAssistantMessageParam) that still annotatetool_callsasIterable[...]and therefore can produce lazy iterators during Python-object validation.Purpose
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.