Convert 'thinking' parameter to 'reasoning_effort' for Claude models - #28031
Convert 'thinking' parameter to 'reasoning_effort' for Claude models#28031coderback wants to merge 5 commits into
Conversation
…t' for Claude models
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge; the conversion logic is correctly scoped to Claude models and the no-overwrite invariant now holds for both param sources. All previously raised blocking concerns have been addressed in this version of the code: non-Claude models are no longer affected, budget_tokens: None is handled via 'or 0', and reasoning_effort already present in optional_params is now checked before conversion. The one remaining inconsistency — the elif branch promoting reasoning_effort for any Claude model without also checking supports_reasoning, unlike get_supported_openai_params — is a low-risk edge case because GitHub Copilot only surfaces reasoning-capable Claude models in practice. No files require special attention for merge safety.
|
| Filename | Overview |
|---|---|
| litellm/llms/github_copilot/chat/transformation.py | Adds map_openai_params override to translate Anthropic thinking → OpenAI reasoning_effort for Claude models routed through GitHub Copilot; logic is sound for the common case, with a minor inconsistency in the elif guard vs get_supported_openai_params. |
| tests/test_litellm/llms/github_copilot/test_github_copilot_transformation.py | Adds 7 new unit tests covering all budget-token tiers, disabled thinking, no-thinking passthrough, and the two guard-invariant cases (reasoning_effort in non_default_params and in optional_params); all tests are mock-only and meet the repo's test-integrity requirements. |
Reviews (11): Last reviewed commit: "fix(transformation): prevent 'reasoning_..." | Re-trigger Greptile
Greptile SummaryThis PR adds a
Confidence Score: 3/5The translation logic works for the happy path, but the guard that is meant to protect an already-set reasoning_effort value is incomplete and can silently overwrite it when the value lives in optional_params rather than non_default_params. The core conversion is correct and tests pass. However, the never-overwrite invariant stated in the PR description is not fully enforced — the check only reads from non_default_params while the method signature also accepts a non-empty optional_params. A caller who has already placed reasoning_effort into optional_params will have it silently replaced by the budget-token mapping, which is the exact behaviour the guard was written to prevent. The tests do not cover this path, so the gap went undetected. litellm/llms/github_copilot/chat/transformation.py — specifically the existing_reasoning_effort guard and the unconditional thinking pop on line 154.
|
| Filename | Overview |
|---|---|
| litellm/llms/github_copilot/chat/transformation.py | Adds map_openai_params override to translate Anthropic thinking to reasoning_effort; the guard for an already-set reasoning_effort only inspects non_default_params, so a value pre-placed in optional_params can be silently overwritten; thinking is also unconditionally dropped for all models, not just Claude models. |
| tests/test_litellm/llms/github_copilot/test_github_copilot_transformation.py | Adds 4 new unit tests covering the main conversion tiers and the "don't overwrite existing reasoning_effort" guard, but is missing a test for the case where reasoning_effort is pre-set in optional_params (the actual bug path). |
Reviews (2): Last reviewed commit: "fix(transformation): convert 'thinking' ..." | Re-trigger Greptile
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 2/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (Greptile gate: score 3/5 below required 4/5 — request a Greptile review ( Fix the issues above and push an update — the bot will re-review automatically.
|
…king' is handled correctly for Claude models
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 2/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (Greptile gate: score 3/5 below required 4/5 — request a Greptile review ( Fix the issues above and push an update — the bot will re-review automatically.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 2/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (Greptile gate: score 3/5 below required 4/5 — request a Greptile review ( Fix the issues above and push an update — the bot will re-review automatically.
|
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 2/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (Greptile gate: score 3/5 below required 4/5 — request a Greptile review ( Fix the issues above and push an update — the bot will re-review automatically.
|
|
@greptileai provide a new summary |
| elif budget_tokens >= 2000: | ||
| reasoning_effort = "low" | ||
| else: | ||
| reasoning_effort = "minimal" |
There was a problem hiding this comment.
The
"minimal" value is not part of the standard reasoning_effort enum recognised by OpenAI-compatible APIs (valid values are "low", "medium", "high"). GitHub Copilot proxies to an OpenAI-compatible endpoint, so sending "minimal" for any request whose budget_tokens falls below 2 000 will likely result in a 400/422 from the Copilot backend. Consider mapping the lowest tier to "low" instead, or verify that Copilot accepts "minimal" before shipping.
| elif budget_tokens >= 2000: | |
| reasoning_effort = "low" | |
| else: | |
| reasoning_effort = "minimal" | |
| elif budget_tokens >= 2000: | |
| reasoning_effort = "low" | |
| else: | |
| reasoning_effort = "low" |
There was a problem hiding this comment.
"minimal" is a valid reasoning_effort value, not an invalid one.
- Confirmed in the installed openai SDK: ReasoningEffort = Literal["none", "minimal", "low", "medium", "high", "xhigh"]. Source: https://github.com/openai/openai-python/blob/main/src/openai/types/shared/reasoning_effort.py
- Already used elsewhere in this codebase: litellm/llms/openai/chat/gpt_5_transformation.py:250 explicitly handles "minimal", and litellm/llms/hosted_vllm/chat/transformation.py uses the identical budget_tokens < 2000 → "minimal" mapping. The new override here is consistent with established codebase patterns.
1 similar comment
|
The fix for #25666 was merged. Closing this |
…t' for Claude models
Relevant issues
Fixes #25666
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Type
🐛 Bug Fix
Changes