Skip to content

fix(bedrock-converse): accept Reasoning(effort=..., summary=...) dict for reasoning_effort - #29329

Open
hclsys wants to merge 3 commits into
BerriAI:litellm_internal_stagingfrom
hclsys:fix/bedrock-reasoning-effort-dict-coerce
Open

fix(bedrock-converse): accept Reasoning(effort=..., summary=...) dict for reasoning_effort#29329
hclsys wants to merge 3 commits into
BerriAI:litellm_internal_stagingfrom
hclsys:fix/bedrock-reasoning-effort-dict-coerce

Conversation

@hclsys

@hclsys hclsys commented May 30, 2026

Copy link
Copy Markdown
Contributor

Companion to #28196 — the direct Anthropic path was already fixed (litellm/llms/anthropic/chat/transformation.py accepts both string and dict reasoning_effort, see #25359), but Bedrock Converse's map_openai_params still had the old isinstance(value, str) guard, silently dropping the dict shape OpenAI Responses callers send ({"effort": "low", "summary": "concise"}).

Symptom: reasoning_tokens == 0 on Bedrock Claude after upgrading; same as the direct path before fix.

fix

Same shape coercion as the Anthropic adapter — pull value["effort"] out before dispatch:

elif param == "reasoning_effort" and value is not None:
    effort_value: Any = value
    if isinstance(effort_value, dict):
        effort_value = effort_value.get("effort")
    if not isinstance(effort_value, str):
        continue
    self._handle_reasoning_effort_parameter(...)

proof

$ .venv/bin/python -m pytest tests/test_litellm/llms/bedrock/chat/test_converse_transformation.py -k reasoning_effort -v
[...]
test_reasoning_effort_accepts_dict_shape_on_bedrock_converse[bedrock/converse/us.anthropic.claude-opus-4-7] PASSED
test_reasoning_effort_accepts_dict_shape_on_bedrock_converse[bedrock/converse/us.anthropic.claude-sonnet-4-6] PASSED
test_reasoning_effort_invalid_dict_does_not_crash_or_emit_thinking PASSED
(+ 15 pre-existing reasoning_effort tests)
====================== 18 passed, 113 deselected in 0.21s ======================

scope

Bedrock Converse only. Vertex/Databricks Anthropic-via-partner paths use the direct AnthropicConfig path (which is already fixed), so they pick this up transitively — but I haven't audited the partner-model dispatch on those providers, leaving that as a separate triage if reports come in.

@CLAassistant

CLAassistant commented May 30, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@greptile-apps

greptile-apps Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes Bedrock Converse's map_openai_params to accept reasoning_effort values shaped as a dict ({"effort": "low", "summary": "concise"}), matching the coercion already present in the direct Anthropic path. Previously the isinstance(value, str) guard silently dropped dict-shaped inputs, causing reasoning_tokens == 0 on Bedrock Claude for callers using the OpenAI Responses bridge.

  • The guard is changed to value is not None with an inline dict-to-string extraction (effort_value.get("effort")); any non-string result after coercion is skipped with continue, which is safe because the subsequent if blocks in the loop check other param names and would not match "reasoning_effort" regardless.
  • Two new pure-transformation unit tests are added: a parametrized happy-path test for adaptive Claude 4.6/4.7 models and a defensive test for malformed dicts (missing "effort" key).

Confidence Score: 5/5

Safe to merge — the change is a minimal guard relaxation with matching tests, and the coercion logic is identical to the already-merged Anthropic direct-path fix.

The fix is narrow: one guard expression replaced with an equivalent dict-unpacking pattern lifted directly from litellm/llms/anthropic/chat/transformation.py. The continue in the inner branch only skips param-specific checks that would not match "reasoning_effort" anyway, so no existing code path is affected. New tests cover both the happy path and the malformed-dict edge case, and no existing tests were weakened.

No files require special attention.

Important Files Changed

Filename Overview
litellm/llms/bedrock/chat/converse_transformation.py Relaxes the reasoning_effort guard from isinstance(value, str) to value is not None, adding dict-to-string coercion that mirrors the existing Anthropic direct-path handler; logic is correct and minimal.
tests/test_litellm/llms/bedrock/chat/test_converse_transformation.py Adds two new unit tests: one parametrized happy-path test for dict-shaped reasoning_effort on adaptive Claude 4.6/4.7 models, and one defensive test for a malformed dict (missing "effort" key). No existing tests modified, no network calls made.

Reviews (1): Last reviewed commit: "fix(bedrock-converse): accept Reasoning(..." | Re-trigger Greptile

@codspeed-hq

codspeed-hq Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 16 untouched benchmarks


Comparing hclsys:fix/bedrock-reasoning-effort-dict-coerce (afc5085) with main (a021a5b)

Open in CodSpeed

@codecov

codecov Bot commented May 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...tellm/llms/bedrock/chat/converse_transformation.py 88.88% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@krrish-berri-2

Copy link
Copy Markdown
Contributor

@hclsys — two quick things before this gets reviewed:

  1. The LiteLLM Linting / lint check is failing, and it looks related to the Python changes in this PR. Could you take a look and fix the lint issues?
  2. Could you also add a screenshot or short video showing a Bedrock Converse request with a Reasoning(effort=..., summary=...) dict being accepted end-to-end? Visual proof really speeds up review.

Thanks!

@hclsys

hclsys commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

companion PR #29330 (same one-line dict coercion for the Databricks Claude adapter) — both bedrock + databricks have the same #28196 root cause that the direct Anthropic adapter already fixed via #25359. Same coercion, sequential audit of the 3 anthropic-via-X adapters in litellm. Either can merge first; the other is a stand-alone identical pattern in a different file.

@hclsys

hclsys commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

@krrish-berri-2 quick on both:

lint is now green — your earlier comment was on the pre-ruff-fix commit; commit afc508570 extracted _coerce_reasoning_effort to keep map_openai_params under PLR0915, ruff check is passing now (see lint row in current checks).

before/after exercised directly against AmazonConverseConfig.map_openai_params for adaptive Claude 4.7:

from litellm.llms.bedrock.chat.converse_transformation import AmazonConverseConfig
cfg = AmazonConverseConfig()
non_default = {"reasoning_effort": {"effort": "low", "summary": "concise"}}
op = cfg.map_openai_params(non_default_params=non_default, optional_params={},
    model="bedrock/converse/us.anthropic.claude-opus-4-7", drop_params=False)
print(op.get("thinking"), op.get("output_config"))

unpatched main:

None None    # ← dict shape silently dropped, thinking never gets set, reasoning_tokens=0 at runtime

this PR:

{'type': 'adaptive'} {'effort': 'low'}    # ← identical to passing reasoning_effort="low"

and the bare string still works (regression-guarded by test_reasoning_effort_bare_string_still_works in #29330, same pattern):

PATCHED dict: {'thinking': {'type': 'adaptive'}, 'output_config': {'effort': 'low'}}
PATCHED str:  {'thinking': {'type': 'adaptive'}, 'output_config': {'effort': 'low'}}
match: True

@hclsys
hclsys changed the base branch from main to litellm_internal_staging May 31, 2026 03:02
hclsys added 3 commits June 4, 2026 23:34
… for reasoning_effort

The direct Anthropic path at litellm/llms/anthropic/chat/transformation.py
already accepts both the bare string ("low") and the OpenAI Responses
`Reasoning(effort, summary)` dict (BerriAI#25359). Bedrock Converse's
`map_openai_params` still guarded the same parameter with
`isinstance(value, str)`, silently dropping the dict shape and producing
reasoning_tokens == 0 on Bedrock Claude after upgrading.

Coerce the dict to its `effort` string before dispatching to
`_handle_reasoning_effort_parameter`, matching the Anthropic adapter.

Related to BerriAI#28196 (the direct-Anthropic half was fixed earlier).
…params under PLR0915

CI ruff caught `Too many statements (54 > 50)` in map_openai_params after my
inline dict-coercion added 5 statements. Pull the coercion into a static helper
`_coerce_reasoning_effort` so the dispatch reads as one elif again.
@hclsys
hclsys force-pushed the fix/bedrock-reasoning-effort-dict-coerce branch from afc5085 to 923a6cc Compare June 4, 2026 15:34
@hclsys

hclsys commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

redid the branch on top of internal_staging — three commits squashed into the three logic commits, no force-push of new content. waiting on CI.

@kimnamu kimnamu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for closing the Bedrock Converse gap here — I ran the branch locally (923a6ccb) and read through _coerce_reasoning_effort, and it checks out cleanly.

  • Sibling consistency is the strongest part. The helper normalizes to the same bare-string shape the direct Anthropic adapter expects, and keeps the silent-drop-on-malformed semantics identical (if dict → .get("effort"), then drop if not a string). Mirroring the already-merged Anthropic path (rather than introducing a fail-fast that diverges across providers) is the right call — and the docstring spelling that out is a nice touch.
  • Verified the edge cases directly against the helper logic: "low"low, {"effort":"low","summary":"concise"}low, {"summary":"concise"} (no effort)→None, {"effort":None}None, 5None, {}None, NoneNone. No crash on any malformed shape — the OpenAI Responses Reasoning(effort, summary) dict and the defensive cases are all covered.
  • The test genuinely catches the bug. Reverting just the guard back to isinstance(value, str) (keeping the new tests) makes the happy-path parametrized case fail (the dict is dropped → thinking never set); restoring the fix → passes. So it's a real regression guard. The walrus-in-elif to stay under PLR0915 is tidy and the skip is safe.

Minimal, well-mirrored, good regression coverage. I'd love to see it land.

(I'm not a maintainer — just a Bedrock user who reviewed and ran this locally. Prepared with the help of an AI agent (Claude Code), human-verified.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants