address review (P1): only inject reasoning_effort='disable' for providers that accept it

Greptile P1 (PR #26642): Anthropic / Bedrock / OpenAI o-series advertise
reasoning_effort in supported_params but their _map_reasoning_effort layers
only accept low|medium|high|minimal|none/None and raise ValueError /
BadRequestError for the literal "disable". Only Gemini, Vertex AI, Ollama
recognise "disable".

- _think_to_reasoning_effort: take an optional custom_llm_provider arg and
  only set reasoning_effort='disable' when provider is in a small allowlist
  of providers known to accept it (gemini, vertex_ai, vertex_ai_beta, ollama,
  ollama_chat). For other providers, log a debug line and drop think:false
  silently (matches pre-fix behavior on those providers, and avoids the
  runtime error described in the review).
- get_optional_params: thread custom_llm_provider through to the helper.
- New parametrized test test_think_false_dropped_for_providers_that_reject_disable
  covers anthropic / bedrock / openai / azure / vertex_ai_anthropic.
- Updated existing helper tests to pass custom_llm_provider='gemini' so the
  positive-path semantics are still asserted.
- Re-applied prior round of feedback (membership check + verbose_logger.debug
  on think=True drop + falsy-explicit test) on top, GPG-signed.

Tests: 20 passed locally.
This commit is contained in:
skgandikota 2026-04-27 23:04:13 +01:00 committed by skgandikota
parent 66d95e6b39
commit b953bfe243
No known key found for this signature in database
GPG key ID: 9C8C0949390E528F
2 changed files with 80 additions and 4 deletions

View file

@ -3683,6 +3683,7 @@ def _think_to_reasoning_effort(
think_value: Any,
non_default_params: dict,
supported_params: List[str],
custom_llm_provider: Optional[str] = None,
) -> None:
"""
Translate the Ollama-style ``think`` flag (passed directly in a chat
@ -3698,9 +3699,14 @@ def _think_to_reasoning_effort(
- ``think`` falsy (``False`` / ``"false"`` / ``0`` / ``"no"`` / ``"off"``):
set ``reasoning_effort = "disable"`` on ``non_default_params`` -- but only
when (a) the caller has not already supplied ``reasoning_effort`` and
(b) the provider lists ``reasoning_effort`` as a supported param.
Otherwise the flag is silently ignored.
when (a) the caller has not already supplied ``reasoning_effort``, (b) the
provider lists ``reasoning_effort`` as a supported param, and (c) the
provider's transformation layer is known to recognise the literal value
``"disable"`` (currently Gemini / Vertex AI / Ollama). Other providers
(Anthropic, Bedrock, OpenAI o-series, ...) advertise ``reasoning_effort``
but only accept ``low|medium|high|minimal|none|None``; passing
``"disable"`` to them raises ``ValueError`` / ``BadRequestError``. For
those providers, ``think`` is silently dropped (with a debug log).
- ``think`` truthy: ``reasoning_effort`` is left untouched (callers that
want to enable reasoning should pass ``reasoning_effort`` explicitly).
- ``think`` is an unrecognized string: ignored.
@ -3719,12 +3725,38 @@ def _think_to_reasoning_effort(
else:
think_bool = bool(think_value)
# Providers whose `_map_reasoning_effort` (or equivalent) accepts the
# literal "disable" value. Adding to this list is safe; omitting a provider
# that *does* accept it just means `think:false` is silently ignored there.
_PROVIDERS_SUPPORTING_REASONING_DISABLE = {
"gemini",
"vertex_ai",
"vertex_ai_beta",
"ollama",
"ollama_chat",
}
if (
think_bool is False
and not non_default_params.get("reasoning_effort")
and "reasoning_effort" not in non_default_params
and "reasoning_effort" in supported_params
and (custom_llm_provider or "") in _PROVIDERS_SUPPORTING_REASONING_DISABLE
):
non_default_params["reasoning_effort"] = "disable"
elif think_bool is False:
verbose_logger.debug(
"litellm: 'think=False' was passed but provider %r does not accept "
"reasoning_effort='disable'; dropping silently. Pass "
"reasoning_effort explicitly if your provider supports a "
"'disable'-equivalent value.",
custom_llm_provider,
)
elif think_bool is True:
verbose_logger.debug(
"litellm: 'think=True' was passed but is not an OpenAI-compatible "
"param; dropping silently. Pass 'reasoning_effort' explicitly to "
"enable reasoning."
)
class PreProcessNonDefaultParams:
@ -4126,6 +4158,7 @@ def get_optional_params( # noqa: PLR0915
think_value=_think_value,
non_default_params=non_default_params,
supported_params=supported_params,
custom_llm_provider=custom_llm_provider,
)
_check_valid_arg(

View file

@ -24,6 +24,7 @@ class TestThinkToReasoningEffortHelper:
think_value=False,
non_default_params=non_default_params,
supported_params=supported_params,
custom_llm_provider="gemini",
)
assert non_default_params["reasoning_effort"] == "disable"
@ -36,6 +37,7 @@ class TestThinkToReasoningEffortHelper:
think_value=falsy,
non_default_params=non_default_params,
supported_params=["reasoning_effort"],
custom_llm_provider="gemini",
)
assert non_default_params["reasoning_effort"] == "disable"
@ -47,6 +49,7 @@ class TestThinkToReasoningEffortHelper:
think_value=True,
non_default_params=non_default_params,
supported_params=["reasoning_effort"],
custom_llm_provider="gemini",
)
assert "reasoning_effort" not in non_default_params
@ -60,6 +63,7 @@ class TestThinkToReasoningEffortHelper:
think_value=False,
non_default_params=non_default_params,
supported_params=["temperature", "top_p"],
custom_llm_provider="cohere_chat",
)
assert "reasoning_effort" not in non_default_params
@ -71,10 +75,48 @@ class TestThinkToReasoningEffortHelper:
think_value=False,
non_default_params=non_default_params,
supported_params=["reasoning_effort"],
custom_llm_provider="gemini",
)
assert non_default_params["reasoning_effort"] == "high"
def test_think_false_respects_falsy_explicit_reasoning_effort(self):
"""An explicit ``reasoning_effort=""`` is a present (if empty) value and
must NOT be overwritten by ``think=False``. Guards against the falsy
membership-vs-presence trap."""
non_default_params: dict = {"reasoning_effort": ""}
_think_to_reasoning_effort(
think_value=False,
non_default_params=non_default_params,
supported_params=["reasoning_effort"],
custom_llm_provider="gemini",
)
assert non_default_params["reasoning_effort"] == ""
@pytest.mark.parametrize(
"provider",
["anthropic", "bedrock", "openai", "azure", "vertex_ai_anthropic"],
)
def test_think_false_dropped_for_providers_that_reject_disable(self, provider):
"""Providers whose `_map_reasoning_effort` does not accept the literal
``"disable"`` (Anthropic, Bedrock, OpenAI o-series, ...) would raise a
runtime ``ValueError`` / ``BadRequestError`` if we injected
``reasoning_effort="disable"``. For these providers, ``think:false``
must be silently dropped instead. Regression test for the P1 review
comment on PR #26642."""
non_default_params: dict = {}
_think_to_reasoning_effort(
think_value=False,
non_default_params=non_default_params,
supported_params=["reasoning_effort"],
custom_llm_provider=provider,
)
assert "reasoning_effort" not in non_default_params
def test_think_unrecognized_string_is_ignored(self):
non_default_params: dict = {}
@ -82,6 +124,7 @@ class TestThinkToReasoningEffortHelper:
think_value="maybe",
non_default_params=non_default_params,
supported_params=["reasoning_effort"],
custom_llm_provider="gemini",
)
assert "reasoning_effort" not in non_default_params