Skip to content

Commit 610e392

Browse files
committed
fix(h2o): require both a configured ceiling and an Azure route, not either
Whether an Azure deployment wants max_tokens or max_completion_tokens is a function of its api_version, not of it being Azure, and the same model name can differ between deployments. An entry can state it explicitly: use_max_completion_tokens: false, which is how an older api_version deployment is expressed, generates max_tokens as the ceiling with NO max_completion_tokens and NO drop. Treating the Azure route alone as sufficient renamed the caller's value on exactly those deployments, overriding a deliberate operator choice. The predicate now fires on the drop list, or on a configured max_completion_tokens AND an Azure route. Both of the latter are needed: the ceiling alone is set for every reasoning model, Azure or not, and firing on a non-Azure provider strips max_tokens from one that needs it; the route alone hits the case above. Requiring both also keeps reasoning Azure entries covered, which carry a ceiling but are exempt from the drop. Verified against the actual output of convert_model_to_litellm_config for four deployment shapes rather than hand-written kwargs: azure non-reasoning renames, azure reasoning renames, azure with use_max_completion_tokens=false does not, bedrock does not. Tests: 24. The two new ones fail on the previous head. Reported by jon: deployments differ by api_version + model name, not by provider alone.
1 parent d152f56 commit 610e392

2 files changed

Lines changed: 69 additions & 17 deletions

File tree

litellm/integrations/h2o/litellm_max_tokens_rename_hook.py

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -48,17 +48,26 @@
4848
4949
* the call is a chat completion, and
5050
* the request carries a positive integer `max_tokens`, and
51-
* the selected deployment is one that discards or rejects `max_tokens`,
52-
i.e. it lists `max_tokens` in `additional_drop_params`, or it is an Azure
53-
route.
54-
55-
A configured `max_completion_tokens` is deliberately NOT a trigger on its own.
56-
`convert_model_to_litellm_config` sets it for every reasoning model, Azure or
57-
not (`use_completion_tokens = is_reasoning_model or is_azure_provider`), and
58-
only the Azure branch adds the drop. Treating it as a trigger would strip
59-
`max_tokens` from non-Azure providers that need it, leaving those requests with
60-
no ceiling at all, which is worse than the bug being fixed. It also cannot be
61-
distinguished from a caller-supplied value once kwargs are merged.
51+
* the selected deployment lists `max_tokens` in `additional_drop_params`,
52+
OR it has a configured `max_completion_tokens` AND is an Azure route.
53+
54+
Neither of those last two signals is sufficient alone, and they fail in
55+
opposite directions.
56+
57+
`max_completion_tokens` alone is too broad: `convert_model_to_litellm_config`
58+
sets it for every reasoning model, Azure or not
59+
(`use_completion_tokens = is_reasoning_model or is_azure_provider`), and only
60+
the Azure branch adds the drop. Firing on a non-Azure provider would strip
61+
`max_tokens` from one that needs it, leaving the request with no ceiling at
62+
all, which is worse than the bug being fixed.
63+
64+
An Azure route alone is also too broad: whether an Azure deployment wants
65+
`max_tokens` or `max_completion_tokens` depends on its api_version, not merely
66+
on being Azure, and the same model name can differ between deployments. An
67+
entry can say so explicitly, and `use_max_completion_tokens: false` (how an
68+
older api_version deployment is expressed) generates `max_tokens` as the
69+
ceiling with NO `max_completion_tokens` and NO drop. Renaming there would
70+
override a deliberate operator choice.
6271
6372
Deployments that natively accept `max_tokens` (Anthropic, Bedrock, vLLM,
6473
non-2025 Azure) are left untouched, including when they share a model group
@@ -150,15 +159,38 @@ def _deployment_discards_max_tokens(kwargs: Dict[str, Any]) -> bool:
150159
Read straight off the merged kwargs rather than the router, so a mixed
151160
model group is judged per selected deployment instead of per group.
152161
"""
162+
# The explicit signal: convert_model_to_litellm_config decided this
163+
# deployment discards max_tokens, so the caller's value can only
164+
# survive as max_completion_tokens.
153165
drop = kwargs.get("additional_drop_params") or []
154166
if isinstance(drop, (list, tuple)) and "max_tokens" in drop:
155167
return True
156-
# Azure 2025+ rejects max_tokens outright. Reasoning Azure entries are
157-
# exempt from the drop above but still need the rename, so recognise
158-
# the route itself. custom_llm_provider is not always populated at this
159-
# point, so the prefixed model string is the primary signal.
168+
169+
# Otherwise require BOTH signals: a configured max_completion_tokens
170+
# AND an Azure route. Either one alone is too broad, in opposite
171+
# directions:
172+
#
173+
# max_completion_tokens alone -- convert_model_to_litellm_config sets
174+
# it for every reasoning model, Azure or not, and only the Azure
175+
# branch adds the drop. Firing on a non-Azure provider would strip
176+
# max_tokens from one that needs it, leaving no ceiling at all.
177+
#
178+
# Azure route alone -- whether an Azure deployment wants max_tokens
179+
# or max_completion_tokens depends on its api_version, not just on
180+
# being Azure, and the entry can say so explicitly. An entry with
181+
# `use_max_completion_tokens: false` (how an older api_version
182+
# deployment is expressed) generates max_tokens as the ceiling, NO
183+
# max_completion_tokens and NO drop. Renaming there would override a
184+
# deliberate operator choice.
185+
#
186+
# Requiring both leaves that case alone while still covering reasoning
187+
# Azure entries, which carry a ceiling but are exempt from the drop.
188+
if kwargs.get("max_completion_tokens") is None:
189+
return False
160190
if kwargs.get("custom_llm_provider") == "azure":
161191
return True
192+
# custom_llm_provider is not always populated at this point, so the
193+
# prefixed model string is the primary Azure signal.
162194
model = kwargs.get("model")
163195
return isinstance(model, str) and model.startswith("azure/")
164196

tests/test_litellm/integrations/h2o/test_max_tokens_rename_hook.py

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,15 +138,35 @@ async def test_malformed_drop_params_never_propagates(hook):
138138

139139

140140
@pytest.mark.asyncio
141-
async def test_azure_route_alone_triggers_the_rename(hook):
141+
async def test_azure_route_with_a_ceiling_triggers_the_rename(hook):
142142
"""Reasoning Azure entries get max_completion_tokens but are exempt from
143-
the drop, so the route itself has to be recognised."""
143+
the drop, so route + ceiling has to be recognised without one."""
144144
out = await _run(hook, {"model": "azure/gpt-5-mini", "max_tokens": 50,
145145
"max_completion_tokens": 16384})
146146
assert out["max_completion_tokens"] == 50
147147
assert "max_tokens" not in out
148148

149149

150+
@pytest.mark.asyncio
151+
async def test_azure_deployment_that_wants_max_tokens_is_untouched(hook):
152+
"""An Azure entry with `use_max_completion_tokens: false`, which is how an
153+
older api_version deployment is expressed, generates max_tokens as the
154+
ceiling with no max_completion_tokens and no drop. Whether Azure wants
155+
max_tokens or max_completion_tokens is a function of api_version, not of
156+
being Azure, so treating the route alone as sufficient would override a
157+
deliberate operator choice on exactly these deployments."""
158+
out = await _run(hook, {"model": "azure/gpt-4o-mini", "max_tokens": 50,
159+
"api_version": "2024-02-01"})
160+
assert out is None
161+
162+
163+
@pytest.mark.asyncio
164+
async def test_azure_route_without_a_ceiling_or_drop_is_untouched(hook):
165+
"""Same rule stated directly on the predicate: route alone is not enough."""
166+
out = await _run(hook, {"model": "azure/gpt-4o-mini", "max_tokens": 50})
167+
assert out is None
168+
169+
150170
@pytest.mark.asyncio
151171
async def test_non_azure_ceiling_alone_does_not_trigger_the_rename(hook):
152172
"""convert_model_to_litellm_config sets max_completion_tokens for EVERY

0 commit comments

Comments
 (0)