[Frontend] Improve error message when tool_choice=required hits max_tokens#36883
Conversation
…okens When tool_choice='required' is set and the model exhausts max_tokens before producing a complete tool call, users previously got a cryptic JSON parse error. This adds clear, actionable error messages that guide users to increase max_tokens. Fixes vllm-project#36794 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chenwenxiaolive
requested review from
DarkLight1337,
aarnphm,
chaunceyjiang and
russellb
as code owners
|
This pull request has merge conflicts that must be resolved before it can be |
|
👋 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 You ask your reviewers to trigger select CI tests on top of 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 If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
|
see #36841 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error messaging for truncated tool calls when tool_choice='required'. The changes add an early check for finish_reason='length' and a fallback for JSON parsing errors, which is a good approach. I have one high-severity suggestion regarding test coverage for the new logic to ensure its robustness.
| if (request.tool_choice == "required" | ||
| and output.finish_reason == "length"): | ||
| raise ValueError( | ||
| "Model reached the max token limit " | ||
| "(finish_reason='length') before generating " | ||
| "complete tool calls, but tool_choice='required' " | ||
| "demands valid tool calls. Please increase " | ||
| "`max_tokens` to allow the model to finish " | ||
| "generating the tool call." | ||
| ) |
There was a problem hiding this comment.
This is a good addition for providing a clear error message. However, this new logic path is not covered by automated tests. The new test in test_tool_choice_required.py only covers the fallback mechanism in _parse_tool_calls_from_content. To prevent future regressions, it's important to add a unit test that specifically triggers this early check. This would likely involve mocking self.engine.generate to return a result with output.finish_reason='length' and asserting that the correct ValueError is raised.
There was a problem hiding this comment.
Code Review
This pull request improves error handling for cases where tool_choice='required' is used and the model fails to produce valid tool call JSON, particularly when hitting a token limit. It introduces an early check for finish_reason='length' in chat_completion_full_generator and adds a more descriptive error in _parse_tool_calls_from_content when JSON parsing fails. While the changes are a good improvement, there is a logical inconsistency between the two new error handling paths that results in a potentially misleading error message. I've suggested a change to address this.
| raise ValueError( | ||
| "When tool_choice='required', the model must produce " | ||
| "valid JSON tool calls, but the generated content " | ||
| "could not be parsed. This can happen when the model " | ||
| "hits the max token limit (finish_reason='length') " | ||
| "before completing the tool call. Consider increasing " | ||
| "`max_tokens` to allow the model to finish generating " | ||
| "the tool call." | ||
| ) from e |
There was a problem hiding this comment.
The new pre-check in chat_completion/serving.py specifically handles the finish_reason='length' case. This means when this except block is reached from the chat_completion_full_generator call path, the finish reason is known not to be length. Therefore, the part of this error message suggesting the cause could be max token limit (finish_reason='length') is misleading in this primary call path. The error message should be made more general to reflect that the cause is a JSON parsing failure for other reasons. Note that this change will require updating the assertion in test_parse_tool_calls_incomplete_json_gives_clear_error.
raise ValueError(
"When tool_choice='required', the model must produce "
"valid JSON tool calls, but the generated content "
"could not be parsed. This may be due to the model "
"failing to follow tool use instructions or generating "
"malformed JSON."
) from e2 participants
Summary
tool_choice='required'is set and the model exhaustsmax_tokensbefore producing a complete tool call, users previously received a cryptic JSON parse error (Invalid JSON: EOF while parsing a string). This PR adds clear, actionable error messages that guide users to increasemax_tokens.chat_completion_full_generator()that detectsfinish_reason='length'withtool_choice='required'and raises a descriptive error before attempting JSON parsing.validate_json()in_parse_tool_calls_from_content()with a try-except to provide a helpful error message as a fallback.Fixes #36794
Test plan
test_parse_tool_calls_incomplete_json_gives_clear_errorverifying the improved error messagetool_choice="required"with a lowmax_tokensand confirm the new error message appears🤖 Generated with Claude Code