Repository navigation
[integrations][anthropic] Stop sending rejected sampling parameters - #1037
Conversation
3ad8bf2 to
178c0f5
Compare
| Object temperature = modelParams.remove("temperature"); | ||
| if (temperature instanceof Number) { | ||
| if (temperature instanceof Number | ||
| && sendSamplingParam(modelName, "temperature", temperature)) { |
There was a problem hiding this comment.
Thanks for fixing this! When both the top-level temperature and additional_kwargs.temperature are set, the latter should take precedence. However, the warning is emitted for the top-level value first and may report the overridden value. Could we resolve the effective temperature before filtering/logging and add a test for this case?
There was a problem hiding this comment.
Yes, good catch. The warning is owed once per model and parameter, so the top-level value claimed it first and named a temperature that was about to be overridden anyway.
Fixed in daa13b34. additional_kwargs is read before the gate now, and effectiveTemperature picks the winner, so the warning names the value the request would otherwise have carried. The temperature case in the switch stays, just empty: dropping the label would send the value through the default branch and onto the body raw.
The test was the awkward part. On a rejecting model neither value is sent, so the only place the choice shows up is the warning text, and the module has no logging binding. What I settled on was passing both temperatures as a Number that records whether toString() ran, since the warning is built with %s. The value that got rendered is the value that got named. Putting the old order back fails that test and nothing else.
There's also a parameterized test on effectiveTemperature for the cases where the top-level value still wins: no temperature key, or one holding a non-number.
Claude Opus 4.7 and later, and the Sonnet, Fable and Mythos 5 lines, answer a non-default temperature, top_p or top_k with a 400. Sending the provider default or omitting the parameter stays acceptable everywhere, so the parameter is dropped rather than clamped: substituting the default would quietly change sampling behavior instead of leaving it to the provider. All three parameters are gated, on every path that reaches the request. In Java that is the top-level temperature read and the additional_kwargs switch, where a key without a case label is forwarded as a raw body property and so reaches the API ungated. Python gates the merged kwargs above its single create call. Adding a temperature case to that switch means an additional_kwargs value now takes precedence over the top-level one. Previously the two were emitted as a typed field and a raw property under the same key, producing duplicate JSON keys, which is undefined. The typed and raw routes serialize identically for a number, so accepting models see no change. The warn-once bookkeeping is keyed on the model and the parameter rather than the model alone. Keyed on the model, a second parameter dropped for a model that had already warned was discarded silently. Generated-by: Claude Code 2.1.234 (Claude Opus 5)
… carried An additional_kwargs temperature takes the top-level one's place, but the top-level value was gated first. The report owed for dropping a sampling parameter is spent once per model and parameter, so it named the value the map was about to override instead of the one the request would have sent. Resolve the two before gating, so the one report names the value actually withheld. The additional_kwargs case label stays as a no-op: without a label of its own the key falls to the default branch and reaches the body as a raw property, ungated. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
…ional_kwargs The stop_sequences arm of the additional_kwargs switch had no test. Deleting its case label sends the value through the default branch as a raw body property and leaves the typed field empty, which nothing caught. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
3c4b75e to
12a44eb
Compare
|
@wenjin272 Thanks for the review. Rebased onto main and pushed. |
…pache#1037) Generated-by: Claude Code 2.1.251 (Claude Opus 5)
Linked issue: #1036
Purpose of change
Claude Opus 4.7+ and the Sonnet, Fable and Mythos 5 lines return HTTP 400 for a non-default
temperature,top_portop_k. The Anthropic integration defaultstemperatureto0.1and sent it on every request without checking the model, so picking one of those models turned every chat call into a provider error. Both languages had the defect.All three parameters are now dropped on models that reject them, in both languages. Dropped rather than clamped: omitting the parameter is accepted everywhere, while substituting the provider default would quietly change sampling behaviour. Each drop is logged once per model and parameter.
Three paths could reach the wire, and all three are gated:
temperatureadditional_kwargs→top_p/top_kadditional_kwargs→temperaturecaselabel, so forwarded as a raw body propertyPython gates the merged kwargs immediately above its single
createcall.The model list is deliberately separate from the prefill and structured-output lists in this connection: the three boundaries do not coincide. Structured output starts at 4.5, prefill rejection at 4.6, sampling rejection at 4.7, so Claude 4.6 rejects a prefill while still accepting a temperature. A test pins that.
One behaviour change. A caller setting
temperatureboth at the top level and inadditional_kwargsnow has theadditional_kwargsvalue win. Previously both were emitted under the same key, producing{"temperature":0.1,"temperature":0.5}, which is undefined. Typed and raw routes serialize identically for a number, so accepting models see no change on the wire. The dropped-parameter warning names the value that was actually withheld: the two routes are resolved before the gate rather than reporting whichever was read first.One bug fix beyond the 400. The warn-once bookkeeping was keyed on the model alone, so a second parameter dropped for an already-warned model was discarded silently. It is now keyed on model and parameter.
Tests
Java 90 in
AnthropicChatModelConnectionTest, 92 in the module. Python 84 passed, 2 skipped. Spotless, Ruff and RAT clean.Coverage was driven by mutation testing. Every mutant in scope dies in both languages: the guard removed per parameter, each parameter's routing removed, and the warn key reverted to model-only.
Worth knowing for future edits: an unrecognised key is forwarded as a raw body property, so asserting the typed accessor is empty does not prove a parameter was left off. The drop tests assert the raw body too.
Which value the warning names is pinned by passing both temperatures as a
Numberthat records whether it was rendered, since the message is assembled with%s.stop_sequencesis covered too, the last arm of theadditional_kwargsswitch that had no test.Not covered: the
LOG.warncall itself, and a rewrite that keeps the dedup helper but rebuilds the message at the call site and logs unconditionally. Both need a logging binding in a module that has none.API
No public API change.
temperature,top_pandtop_kkeep their names, types and defaults; only whether they reach the provider depends on the model. No change on models that accept sampling parameters, including the defaultclaude-sonnet-4-20250514.Documentation
doc-neededdoc-not-neededdoc-includedchat_models.md: bothtemperaturerows note the parameter is not sent on Claude 4.7+; the Javaadditional_kwargsrow addstemperatureand states the precedence. The Pythonadditional_kwargsrow was removed because that parameter does not exist in the Python setup.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.234, 2.1.251 (Claude Opus 5)