[Bugfix] Fix KeyError in parse_response_input for reasoning items with optional content#34499
Conversation
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the KeyError in parse_response_input by implementing a more robust content retrieval mechanism for reasoning items. The new tests cover various scenarios, ensuring the fix handles cases with and without content, and with None content, falling back to summary as expected. The changes improve the reliability of the parse_response_input function when dealing with optional fields in the OpenAI spec.
| parse_chat_output, | ||
| parse_input_to_harmony_message, | ||
| parse_output_message, | ||
| parse_response_input, |
There was a problem hiding this comment.
The parse_response_input function is imported but not used in the TestCommonParseInputToHarmonyMessage class, which tests functions common to both Chat Completion and Responses API. This import is only relevant for TestParseResponseInputReasoningItem.
| parse_response_input, | |
| parse_chat_output, | |
| parse_input_to_harmony_message, | |
| parse_output_message, | |
| ) |
|
Hi @jeonsworld, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
340245c to
53c7cb7
Compare
|
LGTM |
There was a problem hiding this comment.
Thanks for the PR! I'm not a maintainer for this area, but am very familiar with conversion of things to/from the Harmony format and fixed a lot of issues here on the Chat Completions side, and would like to request a few changes to better match the Harmony format conversion for these items.
First, I don't think we should fall back to the summary content here. That summary is not actually something generated directly by the model in its Harmony output, and not something vLLM ever creates. This field is more typically generated by SaaS services as a separate step to provide user-safe reasoning summaries since the raw Chain of Thought is not meant to be shown to the user.
Second, the reasoning items can actually be more than 1 here. Since you're now not asserting they are exactly one, we should use all of them found and not only the first. It's probably reasonable for now to just concatenate the reasoning items together in a string into a single Harmony Message, since this method expects to return a single Message from the single ResponseInputOutputItem.
Third, we need to set the Harmony channel (using msg.with_channel) to analysis since these were originally output to that analysis channel in the original response.
Lastly, if this is actually empty, we should not append an analysis message at all. That means we may need to modify this method to allow returning None and adjust the caller in vllm/entrypoints/openai/responses/serving.py to not append this if it returns None. Or, we could optionally modify this to return a list of Message items (instead of a single), so that we could return an empty list or return a list with multiple (for the case of more than 1 reasoning item) and then adjust the caller to append all items from this list to its message stack instead of appending a single.
If you aren't comfortable making these changes, then I can instead turn this into a separate issue so we can knock these items out in a different PR. But, since you're in here and making this better, I'm pointing these out.
Thanks!
53c7cb7 to
31c085e
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
31c085e to
67e53d9
Compare
|
Thanks for the detailed review! I've addressed all your feedback @bbrowning :
|
|
Hi @jeonsworld, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
…h optional content Per the OpenAI spec, ResponseReasoningItem.content is Optional[List[Content]] = None. Clients like langchain-openai may omit this field when constructing multi-turn input from previous responses, causing a KeyError. Changes based on reviewer feedback: - Remove summary fallback (SaaS-generated, not produced by vLLM) - Concatenate all reasoning content items instead of using only the first - Set Harmony channel to 'analysis' for reasoning messages - Return None for empty/missing content instead of an empty message - Update caller in serving.py to skip None results Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: jeonsworld <jeonsworld@gmail.com>
67e53d9 to
f5f8c04
Compare
bbrowning
left a comment
There was a problem hiding this comment.
Thanks for the updates - the changes and tests look good to me from a Harmony correctness point-of-view. This approval clears my suggested changes status, but this will still need approval from a maintainer to get merged.
To approvers, this fixes a compatibility issue in multi-turn interactions in opencode and codex when it comes to clients passing reasoning information back in. And, it fixes a less obvious correctness/accuracy issue where even when reasoning was getting passed back in it wasn't being properly set to the analysis channel, resulting in us prompting the model suboptimally in multi-turn reasoning scenarios with Harmony models.
|
hi @mgoin should we merge this in? |
|
+1 to merging this - it fixes an important compatibility issues here and there are other subsequent PRs also attempting to fix the same thing. |
…h optional content (vllm-project#34499) Signed-off-by: jeonsworld <jeonsworld@gmail.com> Signed-off-by: Athrael Soju <athrael.soju@gmail.com>
…h optional content (vllm-project#34499) Signed-off-by: jeonsworld <jeonsworld@gmail.com>
…h optional content (vllm-project#34499) Signed-off-by: jeonsworld <jeonsworld@gmail.com>
…h optional content (vllm-project#34499) Signed-off-by: jeonsworld <jeonsworld@gmail.com>
Purpose
Fix
KeyError: 'content'crash inparse_response_input()when a reasoning input item omits the optionalcontentfield during multi-turn Responses API conversations.Per the OpenAI spec,
ResponseReasoningItem.contentisOptional[List[Content]] = None. Clients likelangchain-openaiomit this field when constructing multi-turn input from previous responses, causing:This fix uses
.get("content")with fallback tosummarytext, matching the existing defensive pattern already used inutils.py:190-201.Fixes #34496
Related: #33089
Test Plan
4 unit tests added:
test_reasoning_with_contenttest_reasoning_without_content_uses_summarytest_reasoning_with_none_content_uses_summarytest_reasoning_without_content_or_summaryTest Result
contentkey omitted"content": null"content": [{"type": "reasoning_text", "text": "..."}]Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.