fix(anthropic_messages): gate sampling params on /v1/messages like /chat/completions - #37868
Conversation
|
@greptileai can you reply here with a score and why that score? |
|
Score: 5/5 Here's why: Fix is correct and complete. The core logic — calling Single source of truth. Rather than duplicating the gating logic, this delegates to the same Tests are thorough and correct. Six cases covering: drop under No issues flagged:
The diff is 17 lines of production code and 86 lines of tests. It's minimal, well-motivated, and correct. |
Greptile SummaryThe PR aligns Anthropic
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/llms/anthropic/experimental_pass_through/messages/utils.py | Reuses the shared Anthropic sampling-capability helper when constructing optional Messages API parameters. |
| tests/test_litellm/llms/anthropic/experimental_pass_through/messages/test_request_optional_param_utils.py | Adds isolated regression coverage for sampling-parameter dropping, preservation, and rejection behavior. |
Reviews (2): Last reviewed commit: "refactor(anthropic_messages): drop redun..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| model=model, | ||
| param=param, | ||
| value=filtered_params.pop(param), | ||
| drop_params=drop_params, |
There was a problem hiding this comment.
filtered_params.pop(param) removes each value before the helper conditionally restores it, making parameter ownership and exception behavior harder to reason about; construct the gated result without destructively modifying the dictionary.
Context Used: CLAUDE.md (source)
There was a problem hiding this comment.
The chat path does the same pop-then-reapply for top_k in transformation.py. The helper only writes back supported params, so the pop is required.
There was a problem hiding this comment.
You're right. _apply_sampling_param takes value as an explicit parameter and only writes it back to optional_params when the param is supported — so the caller must pop before passing. The chat path uses the identical pattern. My concern was unfounded; the code is correct.
Apologies for the noise.
| # claude-opus-4-7 removed sampling params (supports_sampling_params: false | ||
| # in the model map) — with drop_params they must be stripped instead of | ||
| # forwarded raw (the API 400s on them). | ||
| original = litellm.drop_params |
There was a problem hiding this comment.
These tests repeatedly assign the process-wide litellm.drop_params setting even though the utility accepts drop_params directly, adding unnecessary shared-state coupling and manual restoration; pass the desired value through the argument instead.
Context Used: CLAUDE.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! |
|
Pushed two follow-up commits on top of the original commit (unchanged, still authored by @mihidumh):
|
…hat/completions /v1/messages forwarded temperature/top_p/top_k raw to models that removed sampling params (supports_sampling_params: false — Claude 4.7+/Fable 5), producing provider 400s that router fallbacks mask as silent model downgrades. The chat path already gates these via AnthropicModelInfo._apply_sampling_param; reuse it in get_requested_anthropic_messages_optional_param so both endpoints agree: drop under drop_params, else raise the clean client-side 400. Fixes #35053
…e global The five tests added here set `litellm.drop_params = True` under a manual try/finally. That trips TQ005 (module-global mutation, 10 new violations over the ceiling) and it leaks process-wide if the finally is ever skipped, which is what the save/restore conftest exists to paper over. `get_requested_anthropic_messages_optional_param` already takes `drop_params` as a kwarg, and that is the path /v1/messages actually uses, so pass it directly. `monkeypatch.setattr` pins the global to False so each test proves the per-request flag alone is sufficient and cannot pass on a leaked global. Verified: TQ gate clean, all 10 tests pass, and the 3 that assert the new gating still fail with the fix in utils.py reverted.
131f2b4 to
1a408d7
Compare
|
Rebased onto staging and dropped the lint commit, now redundant after #37864 merged. @greptileai can you reply here with a score and why that score? |
|
Score: 5/5 Here's why: Fix correctness: The approach is exactly right. Rather than duplicating the gating logic, it reuses No regressions: The Test coverage: 5 targeted unit tests cover all meaningful branches — unsupported model (bare and Scope discipline: Two files changed, both directly on the bug path. No drive-by cleanup, no new abstractions. Live-provider evidence: Real Vertex AI repro (before/after) from the original author, plus the proxy sweep table in #35053. Unusually rigorous for a bugfix PR. The inline 5/5 — ready for maintainer review. |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 1a408d7. Configure here.
The block restated what _supports_sampling_params already documents in its own docstring, and the pyright suppression on the call already names why the private helper is reused.
|
bugbot run |
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 c0118e5. Configure here.
3 participants
Note
This is a copy of #35057 by @mihidumh, pushed to a
litellm_-prefixed branch onBerriAI/litellmso the full CircleCI suite (which does not run on fork PRs) can execute against it. Commit authorship is preserved: the commit is unchanged and still authored by @mihidumh. Full credit for the investigation, fix, and live-provider repro goes to the original authorTLDR
Problem this solves:
/v1/messagesforwardstemperature/top_p/top_kraw to models that removed them/chat/completionsalready drops the same params for the same modelsHow it solves it:
AnthropicModelInfo._apply_sampling_paramgating in the/v1/messagesparam builderdrop_params(matching/chat/completions), else clean client-side 400temperature=1stays allowed everywhere)User Flow
Before: a developer whose app sends a temperature on the Anthropic-native route gets a provider 400, and once fallbacks are configured, a quietly older model instead
drop_paramsturned on, they send POSThttps://litellm-domain/v1/messageswith{"model": "claude-sonnet-5", "max_tokens": 16, "temperature": 0.3, "messages": [{"role": "user", "content": "say ok"}]}`temperature` is deprecated for this model, along with areq_011Ce...request id, so the call was made and rejected upstreamhttps://litellm-domain/v1/chat/completionsand get HTTP 200 with an answer, so the two routes disagree on identical inputdrop_paramsoff and try step 1 again, and the 400 comes back byte for byte identical, because the Anthropic-native route never consulted the setting at alltop_pandtop_kfail the same way, and adding"stream": truereturns the same 400 with no events at all rather than a streammodelfield reveals that the model they asked for was never usedAfter: the same request succeeds with the unsupported sampling params dropped, and the two routes finally agree
drop_paramson, they send the same POSThttps://litellm-domain/v1/messageswith"temperature": 0.3onclaude-sonnet-5top_pgets the same treatment, and"stream": truenow streams normally, ending inmessage_stopat HTTP 200https://litellm-domain/v1/chat/completionswith the same model and temperature still returns HTTP 200, so both routes now behave alikedrop_paramsoff, they instead get an immediate HTTP 400 from the gateway readingclaude-sonnet-5 does not support temperature=0.3. Only temperature=1 is supported., raised before any provider call goes out, so there is no upstream request to pay for or fall back fromclaude-sonnet-4-6, keeps receiving the exact temperature it was sentRelevant issues
Fixes #35053
Original PR: #35057
Linear ticket
Resolves LIT-5989
Type
🐛 Bug Fix
Changes
litellm/llms/anthropic/experimental_pass_through/messages/utils.py: after the existing_maybe_drop_speed_paramgate, runtemperature/top_p/top_kthroughAnthropicModelInfo._apply_sampling_param, the same helper the/chat/completionspath uses, so both surfaces make the same decision from the same source of truthtests/test_litellm/llms/anthropic/experimental_pass_through/messages/test_request_optional_param_utils.py: new coverage for the gatingPre-Submission checklist
uv run pytest tests/test_litellm/<your_test_file>.py -vScreenshots / Proof of Fix
Shared setup for both sides, a proxy booted from the worktree at the named commit against the real Anthropic API, no mocks:
claude-sonnet-5carriessupports_sampling_params: falsein the model map;claude-sonnet-4-6has no such key, so it counts as supporting them and serves as the regression control. Case F re-runs case A against a second proxy booted from the same config withdrop_params: false.Before (dd64331)
A: /v1/messages, unsupported model, temperature
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:37421/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'B: /v1/messages, unsupported model, top_p
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:37421/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"top_p":0.9,"messages":[{"role":"user","content":"say ok"}]}'C: /chat/completions, same model and temperature
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:37421/v1/chat/completions -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'{"id":"chatcmpl-0930850a-e84d-4b00-a5a2-4a15e31e0649",...,"choices":[{"finish_reason":"stop","index":0,"message":{"content":"Ok","role":"assistant",...}}],...}at HTTP 200, the route that already gates correctlyD: /v1/messages, model that supports sampling params (control)
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:37421/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-4-6","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'{"model":"claude-sonnet-4-6","id":"msg_011CeGtD1K5c775fywqgG2WQ",...,"content":[{"type":"text","text":"ok"}],"stop_reason":"end_turn",...}at HTTP 200E: /v1/messages, unsupported model, temperature, streaming
curl -sS -N -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:37421/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"stream":true,"messages":[{"role":"user","content":"say ok"}]}'F: /v1/messages, unsupported model, temperature,
drop_params: falsecurl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:48113/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'`temperature` is deprecated for this modelwithreq_011CeGtFX53Ndr6cN6Q584yG, at HTTP 400. The route never consulteddrop_params, so both settings behave identically hereAfter (c0118e5)
Captured at 1a408d7; the only commit since is
c0118e5b7a, which deletes six comment lines and changes no executable code.A: /v1/messages, unsupported model, temperature
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:22117/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'temperaturekey, so the param was dropped rather than forwardedB: /v1/messages, unsupported model, top_p
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:22117/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"top_p":0.9,"messages":[{"role":"user","content":"say ok"}]}'C: /chat/completions, same model and temperature
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:22117/v1/chat/completions -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'{"id":"chatcmpl-27c9d65c-0021-436c-b79e-5af2da070c49",...,"choices":[{"finish_reason":"stop","index":0,"message":{"content":"Ok","role":"assistant",...}}],...}at HTTP 200, unchanged, so the two routes now agreeD: /v1/messages, model that supports sampling params (control)
curl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:22117/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-4-6","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'{"model":"claude-sonnet-4-6","id":"msg_011CeGtCPBSm1NgZNDm7jp2f",...,"content":[{"type":"text","text":"ok"}],"stop_reason":"end_turn",...}at HTTP 200temperature: 0.3, so supporting models are untouchedE: /v1/messages, unsupported model, temperature, streaming
curl -sS -N -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:22117/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"stream":true,"messages":[{"role":"user","content":"say ok"}]}'message_deltawithstop_reason: end_turn, thenmessage_stopF: /v1/messages, unsupported model, temperature,
drop_params: falsecurl -sS -w '\nHTTP %{http_code}\n' -X POST http://127.0.0.1:48166/v1/messages -H 'Authorization: Bearer sk-qa5989' -H 'Content-Type: application/json' -d '{"model":"claude-sonnet-5","max_tokens":16,"temperature":0.3,"messages":[{"role":"user","content":"say ok"}]}'POST Request Sent from LiteLLMmarkers for the call, so the 400 was raised client-side and nothing reached AnthropicUnit tests
tests/test_litellm/llms/anthropic/experimental_pass_through/messages/test_request_optional_param_utils.pypasses 10/10 at the tip. Reverting onlyutils.pyto the merge base fails 3 of them (test_drop_params_strips_sampling_params_for_unsupported_model,test_drop_params_strips_sampling_params_for_provider_prefixed_model,test_sampling_param_raises_clean_400_without_drop_params), so the new coverage genuinely pins the fix.