[codex] Add Amazon Bedrock Responses support#3326
Conversation
|
@trevorcreech CC for the "early parity/error" detection |
There was a problem hiding this comment.
Main note is we should consider whether we want errors in the client or not. Philosophically putting these on the client will force users to update later as we cut releases which is slightly not ideal imo.
If AWS is going to 400/404 in these cases think it is better to rely on the server-side (similar to azure) so users can seamlessly get those updates without being forced to update their openai lib as AWS makes updates.
| from .._base_client import DEFAULT_MAX_RETRIES | ||
| from ..resources.responses.responses import Responses, AsyncResponses | ||
|
|
||
| BedrockTokenProvider = Callable[[], str] |
There was a problem hiding this comment.
Consider: this is copying azure but may not evolve well to future configurations. See another example like https://github.com/openai/openai-python/blob/main/src/openai/auth/_workload.py#L29-L40
peaceful with this for now though may be better to model within a separate struct and pass that in as config for future config updates
There was a problem hiding this comment.
IIUC this would require a small change in core. If you're cool with this for now, i'll try to keep change limited to bedrock support, but very open to a follow up change.
There was a problem hiding this comment.
Yeah peaceful with following azure for now
| region = aws_region or os.environ.get("AWS_REGION") or os.environ.get("AWS_DEFAULT_REGION") | ||
| if region is None or not region.strip(): | ||
| raise OpenAIError( | ||
| "Must provide one of the `base_url` or `aws_region` arguments, or set the " |
There was a problem hiding this comment.
This seems a little strict but not sure what AWS's expectations are here, consider defaults
There was a problem hiding this comment.
Right now bedrock/mantle only supports regional endpoints. Global is shipping within O(weeks), but I don't know the exact implementation yet.
So I think we need to keep this for now.
There was a problem hiding this comment.
Ack we can loosen later to optional
780bd12 to
afca5f7
Compare
|
Pushed new version |
apcha-oai
left a comment
There was a problem hiding this comment.
Looks good! Only small note is think you will need to readd the workload_identity on copy to keep types happy but other than that looks fine
| region = aws_region or os.environ.get("AWS_REGION") or os.environ.get("AWS_DEFAULT_REGION") | ||
| if region is None or not region.strip(): | ||
| raise OpenAIError( | ||
| "Must provide one of the `base_url` or `aws_region` arguments, or set the " |
There was a problem hiding this comment.
Ack we can loosen later to optional
afca5f7 to
8be32d3
Compare
|
Updated after follow-up: addressed the |
Summary
BedrockOpenAI/AsyncBedrockOpenAIprovider clientsopenai.api_type = "amazon-bedrock"/OPENAI_API_TYPE=amazon-bedrockReview Notes
OPENAI_API_KEY; it readsAWS_BEARER_TOKEN_BEDROCKunless the caller passesapi_keyorbedrock_token_provider.workload_identitystays out of the Bedrock constructors.copy/with_optionsretain the base copy auth keywords for override typing and reject non-Bedrock auth if passed.Testing
46 passedsrc/openai/lib/bedrock.py:0 errorsus-east-2probes with refreshed Bedrock credentials:openai.gpt-5.5pass completed static bearer-token SSE and two token-providerresponses.createcalls/v1/modelsreturns200and listsopenai.gpt-5.4/openai.gpt-5.5/openai/v1/responsesand SDKresponses.createprobes return Bedrock500 internal_server_errorfor bothopenai.gpt-5.4andopenai.gpt-5.5; SDKs surface normalInternalServerErrorwith request ids, so no SDK workaround was addedresponses.connect()still passes through and AWS rejects the websocket handshake with HTTP405