From b953bfe24331863803445dc01681089acb01a5b3 Mon Sep 17 00:00:00 2001 From: skgandikota Date: Mon, 27 Apr 2026 23:04:13 +0100 Subject: [PATCH] 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. --- litellm/utils.py | 41 ++++++++++++++++-- .../test_think_param_normalization.py | 43 +++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/litellm/utils.py b/litellm/utils.py index 2ead2b386a0..bbb00608713 100644 --- a/litellm/utils.py +++ b/litellm/utils.py @@ -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( diff --git a/tests/test_litellm/test_think_param_normalization.py b/tests/test_litellm/test_think_param_normalization.py index afbf3a4bb00..b336f245561 100644 --- a/tests/test_litellm/test_think_param_normalization.py +++ b/tests/test_litellm/test_think_param_normalization.py @@ -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