fix: reasoning forwarding#413
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
![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 introduces support for the reasoning field as an alias for reasoning_content across agentic inference datasets and OpenAI chat messages. It adds validation to ensure both fields are not provided simultaneously, updates the message adapter and types, and includes unit tests. Feedback is provided regarding the resolution of the reasoning value in _chat_message_from_dict, where using the or operator could incorrectly discard empty string values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| def _chat_message_from_dict(msg: dict) -> "ChatMessage": | ||
| """Build a ChatMessage from a dict, forwarding all supported fields.""" | ||
| reasoning_value = msg.get("reasoning_content") or msg.get("reasoning") |
There was a problem hiding this comment.
Using the or operator here can lead to unexpected behavior if reasoning_content is an empty string "" (which is a falsy value in Python). If reasoning_content is "" and reasoning is None or not present, reasoning_value will incorrectly evaluate to None instead of preserving the empty string.\n\nTo make this more robust, explicitly check for None using a conditional expression.
| reasoning_value = msg.get("reasoning_content") or msg.get("reasoning") | |
| reasoning_value = msg.get("reasoning_content") if msg.get("reasoning_content") is not None else msg.get("reasoning") |
There was a problem hiding this comment.
Pull request overview
This PR addresses interoperability between OpenAI-compatible backends/routers by treating reasoning_content (SGLang/DeepSeek) and reasoning (vLLM) as aliases, so requests can reach routers that only accept one of the two.
Changes:
- Add
reasoningto the OpenAIChatMessagemsgspec type and forward bothreasoningandreasoning_contentin outgoing requests. - Enforce an alias policy in the agentic dataset builder to reject rows that contain both fields.
- Add/update unit tests and update the agentic dataset README to document the new field.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/inference_endpoint/openai/types.py |
Adds reasoning field to ChatMessage schema. |
src/inference_endpoint/openai/openai_msgspec_adapter.py |
Forwards reasoning alias fields when building request messages. |
src/inference_endpoint/dataset_manager/agentic_inference_dataset.py |
Rejects dataset rows containing both reasoning aliases and includes reasoning in extracted message fields. |
tests/unit/openai/test_msgspec_adapter.py |
Adds request-side tests for reasoning alias forwarding. |
tests/unit/dataset_manager/test_agentic_inference_dataset.py |
Adds dataset validation tests for reasoning alias policy. |
examples/10_Agentic_Inference/README.md |
Documents reasoning as an allowed dataset field. |
Comments suppressed due to low confidence (1)
src/inference_endpoint/dataset_manager/agentic_inference_dataset.py:182
- When building the per-row message dict, missing values coming from a PyArrow-backed DataFrame can be
pd.NA(notNoneand not a float NaN). Those should be filtered out, otherwisepd.NAmay end up in the emitted message payload.
val = row.get(key)
if val is not None and not (isinstance(val, float) and pd.isna(val)):
msg[key] = val
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _chat_message_from_dict(msg: dict) -> "ChatMessage": | ||
| """Build a ChatMessage from a dict, forwarding all supported fields.""" | ||
| reasoning_value = msg.get("reasoning_content") or msg.get("reasoning") | ||
| return ChatMessage( |
| [ | ||
| ({"reasoning_content": "thinking via sglang"}, "thinking via sglang"), | ||
| ({"reasoning": "thinking via vllm"}, "thinking via vllm"), | ||
| ({"reasoning_content": "sglang", "reasoning": "vllm"}, "sglang"), | ||
| ], | ||
| ids=["reasoning_content", "reasoning", "reasoning_content_precedence"], |
|
|
||
|
|
| for _, row in sorted_group.iterrows(): | ||
| role = row.get("role") | ||
| reasoning_content = row.get("reasoning_content") | ||
| reasoning = row.get("reasoning") | ||
| if ( | ||
| reasoning_content is not None | ||
| and not ( | ||
| isinstance(reasoning_content, float) and pd.isna(reasoning_content) | ||
| ) | ||
| and reasoning is not None | ||
| and not (isinstance(reasoning, float) and pd.isna(reasoning)) | ||
| ): | ||
| raise InputValidationError( | ||
| f"conversation {row.get('conversation_id')!r} turn {row.get('turn')} " | ||
| "has both 'reasoning_content' and 'reasoning' values" | ||
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
=======================================
Coverage ? 80.22%
=======================================
Files ? 129
Lines ? 17288
Branches ? 0
=======================================
Hits ? 13870
Misses ? 3418
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What does this PR do?
Current endpoint sends
reasoning_content, which is accepted by sglang and normalized toreasoninginternally for vllm, but some router frameworks (e.g.: vllm_router) dropsreasoning_contentand only acceptsreasoning. This PR sends bothreasoningandreasoning_contentto the server regardless of which one is present in the dataset and rejects if both are present.Type of change
Related issues
Testing
Checklist