Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughReasoning input parsing ignores missing or falsy ChangesReasoning input validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was identified in the reasoning-input change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The harmony path of _parse_response_input indexed input_msg['content'] and asserted its length on unvalidated client input. Replaying a prior turn with a reasoning item that omits 'content' (the canonical shape) raised KeyError, and an empty list tripped a message-less assert that surfaced as an HTTP 400 with an empty error string. Mirror the non-harmony sibling: default missing content and raise a descriptive ValueError. Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
d87e29d to
e212130
Compare
| assert len(content) == 1 | ||
| content = input_msg.get("content") or [] | ||
| if len(content) != 1: | ||
| raise ValueError( |
There was a problem hiding this comment.
As you have stated, OpenAI Responses API allows reasoning "content" to be missing, and infers it from "id" or recovered from "encrypted_content".
Can you either
- Make it clear in the error message that the support is unimplemented instead of the request being invalid
- Follow the fallback standard behavior to ignore an empty reasoning message
There was a problem hiding this comment.
Thanks @tongyuantongyu — went with the second option in f989376: a reasoning item with missing or empty content is now skipped (with a debug log noting that recovery from id/encrypted_content is unimplemented), matching the standard fallback behavior. The caller already filters None messages, so replayed turns parse cleanly. The descriptive error is kept only for the >1 content elements case.
The Responses API allows reasoning items without 'content' (inferred from 'id' or recovered from 'encrypted_content', which is not implemented here). Instead of rejecting such items as invalid, follow the standard fallback behavior and skip them; the caller already filters None messages. Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
|
It appears to be pre-commit check failures~, could you please take a look? Thank you! |
Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
svc-trtllm-gh-bot
added
the
Community want to contribute
This branch has not been deployed
4 participants
Description
The harmony path of
_parse_response_inputdidcontent = input_msg["content"]plus a bareassert len(content) == 1on unvalidated client input. Replaying a prior turn with a reasoning item that omitscontent— the canonical shape, sincecontentis optional on reasoning items — raisedKeyError: 'content', and an emptycontentlist tripped the message-less assert, surfacing as an HTTP 400 with an empty error string.Mirror the non-harmony sibling (
_response_output_item_to_chat_completion_message): default missing content with.get(...) or []and raise a descriptiveValueError.Test Plan
POST /v1/responseson a harmony (gpt-oss) server withinput: [{"type": "reasoning", "id": "rs_1", "summary": []}]now returns a 400 naming the reasoning item instead ofKeyError/empty message.Dev Engineer Review
_parse_response_inputnow treats missing or falsy reasoningcontentas an empty list, logs a debug message, and ignores the item.ValueErrorwhen more than one element is provided.ValueError.tensorrt_llm/serve/responses_utils.py.git diff --checkreported no whitespace errors. Test results are unavailable.QA Engineer Review
No test changes.