fix(gpt-5): stop forwarding temperature and top_p to reasoning models that reject them - #38593
Conversation
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 9/10. This is a strong, focused fix with no blocking correctness issues found. It correctly separates I’m withholding the final point because the PR’s CI status is still pending, so the complete suite has not yet provided an independent confirmation. |
Greptile SummaryThis PR adds model-catalogue metadata for default reasoning effort and uses it consistently when deciding whether GPT-5 requests may forward sampling parameters.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/llms/openai/chat/gpt_5_transformation.py | Centralizes effective reasoning-effort resolution and applies it to temperature, top_p, and logprobs gating. |
| litellm/llms/openai/responses/transformation.py | Reuses the shared GPT-5 effort resolver when filtering Responses API temperature. |
| litellm/llms/azure/chat/gpt_5_transformation.py | Normalizes Azure model aliases through one cost-map lookup hook. |
| litellm/utils.py | Adds string-valued model metadata lookup while preserving input parameters through separate resolved bindings. |
| model_prices_and_context_window.json | Declares a default reasoning effort of none for verified GPT-5 model entries. |
| tests/test_litellm/llms/openai/test_gpt5_transformation.py | Covers sampling-parameter behavior across explicit, declared-default, undeclared, and older-catalogue cases. |
Reviews (2): Last reviewed commit: "fix(gpt-5): resolve temperature support ..." | Re-trigger Greptile
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f5abf73. Configure here.
f5abf73 to
e649da2
Compare
|
Pushed e649da2 addressing the rebinding finding (replied on the thread; it also corrects the LIT011 detail). CI lint is green locally across all of: ruff check litellm, ruff check --config ruff-tests.toml tests, ruff_strict_gate, type_discipline_gate, test_quality_gate, ruff format, check_files_match and the schema --check. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 10/10. The rebinding concern is resolved in |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e649da2. Configure here.
e649da2 to
3b33bbb
Compare
|
Pushed 3b33bbb. CI's basedpyright budget gate caught two
Both are intentional API surface rather than suppressions. All nine gates green locally now: ruff check litellm, ruff check --config ruff-tests.toml tests, ruff_strict_gate, type_discipline_gate, test_quality_gate, type_check_gate, ruff format, check_files_match, schema --check. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 5/5. The basedpyright findings are resolved in the correct way: both cross-module consumers now use deliberately public APIs rather than private-usage suppressions. |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3b33bbb. Configure here.
…oning effort A gpt-5 model accepts a non-default temperature only while its effective reasoning effort resolves to "none". litellm had no representation of the effort a model applies when the request omits reasoning_effort, so it substituted supports_none_reasoning_effort, which is a different fact. Every model that supports "none" without defaulting to it therefore had temperature forwarded and rejected upstream, and because the carve-out returned before the drop_params branch, drop_params: true could not save it. Declare the fact instead. A new cost-map key, default_reasoning_effort, states the effort the provider applies when the request omits one, and one shared predicate resolves the effective effort from it: an explicit reasoning_effort wins, otherwise the declared default, otherwise the catalogue decides. That last step matters because the cost map is fetched from the published branch at import time, so it can be OLDER than the code reading it. On such a map every model looks undeclared, and reading that as "reasoning is active" would strip temperature from the 39 gpt-5.1/5.2/5.4 entries that accept it, a regression caused by data lag rather than by anything about the model. So an absent declaration is only meaningful once the catalogue carries the key at all; a map that predates the feature keeps the answer litellm gave before it existed, and the conservative answer applies from the moment the data lands. The top_p/logprobs/top_logprobs gate carried the same assumption spelled differently and now shares the predicate, as does the Responses API, which reimplemented the rule and is what the default /v1/messages bridge routes openai models through. Azure normalises its routing names in one resolver that every capability lookup goes through, which replaces its bespoke per-lookup rewrite. Declared on the 37 gpt-5.1/5.2/5.4 entries measured to accept temperature=0 today, so their behaviour is unchanged. The 23 gpt-5.5/5.6 entries that reject it stay undeclared and are fixed once the catalogue carries the key. Resolves LIT-3797 Resolves LIT-5028
3b33bbb to
e5c3df2
Compare
|
Pushed e5c3df2. A base-vs-head live run ( The cost map is fetched from the published Measured against the actually-published map, base vs head, tree identity asserted on both sides:
The 39 were gpt-5.1/5.2/5.4 and their azure variants silently losing The fix: an absent declaration only means something once the catalogue carries the key at all. A map predating the feature keeps the old answer; the conservative answer switches on by itself once the data lands. Costs one Also verified while I was in there: @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 5/5 This revision addresses the regression correctly. The catalogue-version check distinguishes an older map that predates The added coverage exercises current-map and older-map behavior, explicit versus default effort, Responses, Azure aliases, and model-info hydration. The reported live base-vs-head and merge-ref results provide additional evidence that the prior regression is gone. I see no remaining merge-blocking correctness issue. |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e5c3df2. Configure here.
|
Filed LIT-6355 for the originally reported symptom (case study section 7.2: shadow-eval judge returning On the red Those are the pre-existing Codecov reported 92.50% on the two previous shas of this PR and 21.42% on this one. The only difference is that this push inserted a function near the top of |
PR #38593 stopped forwarding temperature to reasoning models, which left test_extra_body_merges_with_request_data raising UnsupportedParamsError and test_bad_request_bad_param_error no longer getting a rejection from OpenAI because drop_params now eats the param. Both repairs are the same hunks PR #38739 carries, so the branches merge clean in either order
TLDR
Problem this solves:
temperature/top_ponly when reasoning effort resolves to "none"drop_paramsbranch, so the provider 400 leaked to callersHow it solves it:
default_reasoning_effortdeclares the effort a provider applies when unsettemperatureandtop_p/logprobsacross chat, Responses, and/v1/messagesdrop_params, else an actionable 400azure/, bare,gpt5_series/) normalise in one resolverUser Flow
Before: an agent or eval job using a gpt-5.6-family model with
temperature: 0dies on a raw provider 400 even though the proxy setsdrop_params: true"model": "gpt-5.6-terra", "temperature": 0Unsupported value: 'temperature' does not support 0 with this model. Only the default (1) value is supported.judged_count: 0with every attempt an error rowAfter: the same requests succeed because the proxy drops the parameter the model cannot take
"model": "gpt-5.6-terra", "temperature": 0temperatureunderdrop_params: truetop_pfollows the same ruletemperatureexactly as beforeRelevant issues
Supersedes #34210
Linear ticket
Resolves LIT-6355
Resolves LIT-3797
Resolves LIT-5028
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Local proxy, real OpenAI upstream, no mocks. Shared config:
Same five commands both runs, only the checked-out commit changes.
Before (49affa7)
/v1/chat/completions gpt-5.6-terra temperature=0
curl -s -X POST http://localhost:31620/v1/chat/completions -H "Authorization: Bearer sk-litellm-qa-6429" -H 'Content-Type: application/json' -d '{"model":"gpt-5.6-terra","messages":[{"role":"user","content":"Say ok"}],"temperature":0}'litellm.BadRequestError: OpenAIException - Unsupported value: 'temperature' does not support 0 with this model. Only the default (1) value is supported./v1/responses gpt-5.6-terra temperature=0
curl -s -X POST http://localhost:31620/v1/responses -H "Authorization: Bearer sk-litellm-qa-6429" -H 'Content-Type: application/json' -d '{"model":"gpt-5.6-terra","input":"Say ok","temperature":0}'litellm.BadRequestError: OpenAIException - Unsupported parameter: 'temperature' is not supported with this model./v1/messages gpt-5.6-terra temperature=0
curl -s -X POST http://localhost:31620/v1/messages -H "Authorization: Bearer sk-litellm-qa-6429" -H 'Content-Type: application/json' -d '{"model":"gpt-5.6-terra","max_tokens":32,"messages":[{"role":"user","content":"Say ok"}],"temperature":0}'litellm.BadRequestError: OpenAIException - Unsupported parameter: 'temperature' is not supported with this model./v1/chat/completions gpt-5.6-terra top_p=0.5
curl -s -X POST http://localhost:31620/v1/chat/completions -H "Authorization: Bearer sk-litellm-qa-6429" -H 'Content-Type: application/json' -d '{"model":"gpt-5.6-terra","messages":[{"role":"user","content":"Say ok"}],"top_p":0.5}'litellm.BadRequestError: OpenAIException - Unsupported parameter: 'top_p' is not supported with this model.control: /v1/chat/completions gpt-5.1 temperature=0
curl -s -X POST http://localhost:31620/v1/chat/completions -H "Authorization: Bearer sk-litellm-qa-6429" -H 'Content-Type: application/json' -d '{"model":"gpt-5.1","messages":[{"role":"user","content":"Say ok"}],"temperature":0}'okAfter (e5c3df2)
/v1/chat/completions gpt-5.6-terra temperature=0
ok/v1/responses gpt-5.6-terra temperature=0
ok/v1/messages gpt-5.6-terra temperature=0
[{"type": "text", "text": "ok"}]/v1/chat/completions gpt-5.6-terra top_p=0.5
okcontrol: /v1/chat/completions gpt-5.1 temperature=0
okThe same rig reproduced the customer-visible symptom end to end. A shadow-evaluation job whose judge was gpt-5.6-terra returned
judged_count: 0with every attempt an error row; on the same job, same key and same requests after the fix:judged_counterror_countjudge_spendresultsThat path never needed a caller-side change:
judge_acompletionalready passesdrop_params=True, and the primitive now honours it.Type
🐛 Bug Fix
Caveats (if any)
Low
supports_none_reasoning_effortwithoutdefault_reasoning_effortdrops the params conservativelyDetails
The bug
A gpt-5 model accepts a non-default
temperatureonly while its effective reasoning effortresolves to
"none". litellm has no representation of the effort a model applies when therequest omits
reasoning_effort, so it substitutedsupports_none_reasoning_effort, which isa different fact. Models that support
"none"without defaulting to it hadtemperatureforwarded and rejected upstream, and because the carve-out returned before the
drop_paramsbranch,
drop_params: truecould not save it.Measured, against the live provider
nonemediumThe cleanest confirmation is a pair from one family, one map flag apart, behaving oppositely:
gpt-5.6-cybercarries nosupports_none_reasoning_effortand litellm correctly droppedtemperature, while
gpt-5.6-terracarries it and litellm forwardedtemperature=0.The fix
Declare the missing fact.
default_reasoning_effortstates the effort the provider applies whenthe request omits one, and a single predicate resolves the effective effort: an explicit
reasoning_effortwins, otherwise the declared default, otherwise the conservative answer.An undeclared model resolves to "reasoning is active", so a model released before its entry
declares a default is dropped or refused with an actionable error instead of 400ing at the
provider, and needs no code change once its entry lands.
Three consumers now share that one predicate:
temperaturegatetop_p/logprobs/top_logprobsgate, which carried the same assumption spelleddifferently and was live-confirmed broken the same way
/v1/messagesbridgeroutes openai models through
Azure normalises its routing names in one resolver every capability lookup goes through, which
replaces its bespoke per-lookup rewrite and makes
azure/, bare andgpt5_series/names agree.Blast radius
60 map entries carry
supports_none_reasoning_effort: true, all gpt-5.x, none other.temperature=0today and are declared"none", so their behaviour is byte-identicalTests
Mutation-checked: with the sources reverted, the new cases fail on exactly the broken models and
pass on the preserved ones. The
azureandresponsessuites needed the bundled cost map pinned,since the default import fetches the published map, which lags this branch.
Notes for review
supports_nonewhen the keyis absent, which is the original guess, so the gpt-5.6 family released after it would still have
been broken. This one inverts the fallback and declares only defaults measured against the
provider, so no unverified effort level enters the shared catalogue.
type-discipline-budget.jsonis already exceeded on staging for LIT002, LIT006 and LIT012. Thisbranch is neutral on every counter; the pre-existing drift is not from here.
Post-review: a regression a live base-vs-head run caught
An earlier revision of this PR fell back conservatively whenever
default_reasoning_effortwasabsent. That is wrong when the CATALOGUE is older than the code, which is the default topology:
litellm/__init__.py:413fetches the cost map from the publishedmainbranch at import time,and that map does not carry a key until this PR lands there.
Measured on the actually-published map, base vs head, both trees asserted:
The 39 were
gpt-5.1,gpt-5.2,gpt-5.4and theirazure/,azure/us|eu|global/andazure_ai/variants silently losingtemperature, models that provably accept it. It also brokea real test,
test_azure_base_model_routing.py::test_should_pass_logprobs_through_get_optional_params,which is unpinned and so reads the published map: it passes on base, failed on the earlier
revision, and passes now. CI did not catch it because the outcome depends on whether the remote
map fetch succeeds or falls back to the bundled copy.
So the absence of a declaration is only meaningful once the catalogue carries the key at all. A
map predating the feature keeps the answer litellm gave before it existed, and the conservative
answer switches on by itself the moment the data lands.
TestACatalogueOlderThanTheCodeDoesNotStripTemperaturepins both halves, mutation-checked.
Merge-ref checked against current staging (
2306816d40): merges clean, 0 changes and 0regressions on the merged tree.