Repository navigation
Conversation
|
| if _tools is not None: | ||
| non_default_params["tools"] = _tools | ||
|
|
||
| output_config: Final = non_default_params.pop("output_config", None) |
There was a problem hiding this comment.
Output effort never reaches mapper Direct
completion() calls discard output_config during preprocessing, so this mapping cannot send the requested effort to hosted vLLM. The new tests bypass preprocessing
Knowledge Base Used: Model invocation runtime
| elif thinking is not None and isinstance(thinking, dict) and thinking.get("type") == "enabled": | ||
| pass |
There was a problem hiding this comment.
Empty branch obscures precedence This branch only executes
pass. Removing it would preserve behavior and make the effort precedence easier to follow
| elif thinking is not None and isinstance(thinking, dict) and thinking.get("type") == "enabled": | |
| pass |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ) | ||
| assert optional_params["reasoning_effort"] == "low" | ||
|
|
||
| # Test output_config.effort is mapped to reasoning_effort |
There was a problem hiding this comment.
Test narration violates comment rule The new comments here and below restate the assertions. The repository prohibits such comments; remove both before merging
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Nice — thanks for picking this up, and your reading of the mapping matches the 1. Dead branch. The final elif thinking is not None and isinstance(thinking, dict) and thinking.get("type") == "enabled":
passThe 2. One question worth settling before merge, and I genuinely do not know the The reason I ask: in if isinstance(output_config, dict) and "effort" in output_config:
residual = {k: v for k, v in output_config.items() if k != "effort"}If that runs before the provider's An end-to-end assertion through the 3. Scope. @alvarogarciapiz asked for "OpenAI-compatible providers such as Happy to help with any of this, including taking the broader layer as a separate |
|
Following up on my point 2 above — I traced it, and I owe you a correction plus The file I originally pointed at is not on this path: a native The concern survives in a different form, though. On that adapter path If that reading is right, this PR fixes a direct I am not trying to talk you out of the PR; I would just hate for #44560 to be |
|
"Cannot merge due to insufficient permissions. Skipping merge as requested." |
|
Thanks @aayushbaluni for the careful review! Cleaned up the dead |
|
any updates? |
Title
fix(hosted_vllm): map output_config effort and adaptive thinking to reasoning_effort
Relevant issues
Fixes #44560
Type
Bug Fix
Description
When calling
/v1/messagesor passing requests with adaptive thinking and output configuration (e.g.thinking: {"type": "adaptive"}andoutput_config: {"effort": "low"}orthinking: {"type": "adaptive", "effort": ...}),HostedVLLMChatConfig.map_openai_paramspreviously only handledthinking.get("type") == "enabled"with token budgets.This PR updates
HostedVLLMChatConfig.map_openai_paramsto:output_configand mapoutput_config["effort"], orthinking["effort"], toreasoning_effortwhenreasoning_effortis not already explicitly specified.output_configandthinkingso OpenAI-compatible hosted vLLM backends receive clean top-levelreasoning_effort.test_hosted_vllm_supports_thinking.Tests
tests/unit/llms/hosted_vllm/chat/test_hosted_vllm_chat_transformation.py