Repository navigation
feat(bedrock)!: bridge OpenAI model function tools with reasoning to native /responses instead of Converse #45609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
e2966ab
30c6687
982825a
cd175d0
99118c3
6b9823a
ad25280
88355aa
45219c2
7808c84
811317c
511f7a6
71983d8
b66028f
222a9bf
29f49eb
229dbaf
3b95b1b
bde4179
bc4ac51
f6873b9
ed30e41
9556fc1
2fb185e
bf3c6f2
240ed79
3613392
e4fe5d6
826af5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5723,7 +5723,11 @@ def completion( | |
| "allowed_openai_params": allowed_openai_params, | ||
| "base_model": base_model, | ||
| } | ||
| optional_params = get_optional_params(**optional_param_args, **non_default_params) | ||
| optional_params = get_optional_params( | ||
| **optional_param_args, | ||
| **non_default_params, | ||
| _litellm_responses_api_bridge_allowed=not skip_responses_api_bridge, | ||
| ) | ||
| processed_non_default_params: Final = pre_process_non_default_params( | ||
| model=model, | ||
| passed_params=optional_param_args, | ||
|
|
@@ -5878,11 +5882,32 @@ def completion( | |
| api_base=api_base, | ||
| ) | ||
|
|
||
| request_params: Final = MappingProxyType( | ||
| _OPTIONAL_PARAMS_ADAPTER.validate_python( | ||
| { | ||
| **optional_param_args, | ||
| **non_default_params, | ||
| "_litellm_responses_api_bridge_allowed": not skip_responses_api_bridge, | ||
| } | ||
| ) | ||
| ) | ||
| bedrock_bridges_to_responses: Final = ( | ||
| custom_llm_provider == "bedrock" | ||
| and bedrock_route_for_request( | ||
| model, | ||
| request_params, | ||
| _ADDITIONAL_DROP_PARAMS_ADAPTER.validate_python(kwargs.get("additional_drop_params") or ()), | ||
| ) | ||
| == "responses" | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| ) | ||
|
|
||
| # Use base_model (the true underlying model) for Azure model-type | ||
| # detection when the deployment name differs from the model name. | ||
| _azure_detection_model: Final = base_model or model | ||
|
|
||
| if responses_api_model_info.get("mode") == "responses" and not skip_responses_api_bridge: | ||
| if ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium · Bridged Bedrock chat calls can't be found in spend logs by the returned id
The PR caveats acknowledge this as "same as OpenAI's bridge". The PR still brings the mismatch to Bedrock deployments that had working lookups, and the integration tests avoid it by querying EvidenceSDK probe (
The PR description's Medium caveat acknowledges this: "Bridged chat calls log as Responses calls, so This conclusion rests on code inspection plus the SDK logging probe. I did not query a proxy DB row by the returned id. Latest summary by code-review · View this review · Reviewed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OpenAI and Azure bridged chat log the same encoded id on main, so the fix belongs in the shared bridge as a separate follow-up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding still applies The reply correctly says the id mismatch lives in the shared chat-to-Responses bridge, which OpenAI and Azure already use on main. Fixing it there in a follow-up is reasonable. That fact does not refute this finding, which covers a regression for Bedrock deployments.
The unchanged code in head still shows this mismatch. The bridged Bedrock tests also avoid it by using Evidence
Files changed by this PR: The PR description lists this as a known Medium caveat. Acknowledging a limitation does not resolve it. The finding's original SDK probe showed a returned id of Reviewed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now a listed Decision. Changing the shared bridge's logged or returned id also changes OpenAI and Azure, so a maintainer decides There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding still applies Thanks for adding this to the Decisions list. The new entry documents the tradeoff and leaves the choice to a maintainer, but it doesn't change the code or show that the finding is wrong. The defect is still in head
I agree the right fix may belong in the shared bridge and may need a maintainer's call because it affects OpenAI and Azure. Until that fix lands, or this PR keeps Bedrock's logged and returned ids consistent, the finding stays open. A maintainer can still accept it as a known regression. EvidenceThe head diff touches
The PR description lists this under Decisions ("Bridged chat calls log as Responses calls ... with ... an encoded I did not rerun the earlier SDK probe for this reply because head leaves the cited id paths unchanged. Reviewed |
||
| responses_api_model_info.get("mode") == "responses" or bedrock_bridges_to_responses | ||
|
veria-ai[bot] marked this conversation as resolved.
Resolved
|
||
| ) and not skip_responses_api_bridge: | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
greptile-apps[bot] marked this conversation as resolved.
Resolved
greptile-apps[bot] marked this conversation as resolved.
Resolved
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| from litellm.completion_extras import responses_api_bridge | ||
|
|
||
| optional_params, rs_val = strip_reasoning_summary_aliases_from_optional_params(optional_params) | ||
|
|
@@ -5950,9 +5975,7 @@ def completion( | |
| optional_params=optional_params, | ||
| organization=organization, | ||
| provider_config=provider_config, | ||
| request_params=MappingProxyType( | ||
| _OPTIONAL_PARAMS_ADAPTER.validate_python({**optional_param_args, **non_default_params}) | ||
| ), | ||
| request_params=request_params, | ||
| shared_session=shared_session, | ||
| stream=stream, | ||
| temperature=temperature, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The claim “Async calls take the shared chat-to-Responses bridge” relies on live QA from
222a9bf827, before the stream fix and main merge. The description says current verification uses unit tests. Repository rules require live proof at this tip before merging. Start the current proxy and repeat real Bedrock requests through/v1/chat/completions,/v1/messages, and/v1/responses, including streaming and a tool-result follow-upRule Used: Apply this rule to every PR, including PRs that change only tests, docs, or catalog data. It does not require new live tests for test-only PRs; it only checks that each runtime claim the PR makes is backed by evidence at this PR's tip A runtime clai... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, live QA at this tip waits on a Bedrock credential, and it runs again on all three endpoints before this PR merges