-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Support us-gov. and other multi-character Bedrock geo prefixes
#3645
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
Support us-gov. and other multi-character Bedrock geo prefixes
#3645
Conversation
dsfaccini
left a comment
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.
thank you for the PR @Leundai ! I've requested a couple changes, let me know if you have any questions.
The BedrockProvider.model_profile method previously only handled 2-character regional prefixes (e.g., 'us.', 'eu.'), causing issues with models using longer prefixes like 'us-gov.' (AWS GovCloud) and 'global.'. This caused extended thinking to fail in multi-turn conversations because bedrock_send_back_thinking_parts stayed at its default False value for these models. ThinkingPart blocks from previous turns were converted to text blocks instead of reasoningContent, causing Bedrock to reject requests with: 'Expected thinking or redacted_thinking, but found text' Changes: - Add _strip_geo_prefix() helper function to properly handle all known geo prefixes including us-gov. and global. - Update _AWS_BEDROCK_INFERENCE_GEO_PREFIXES to include us-gov. - Add comprehensive tests for all geo prefixes Fixes cross-region inference for AWS GovCloud environments.
… BEDROCK_GEO_PREFIXES This test ensures we don't add new model names with geo prefixes that aren't handled by the provider's model_profile method. Also exports BEDROCK_GEO_PREFIXES in __all__.
Model names with geo prefixes have 3+ dot-separated parts, so we can detect them without knowing the provider names.
53d0013 to
745e2eb
Compare
Updated the docstring for the test_bedrock_provider_model_profile_all_geo_prefixes function to be more concise. Modified the error message in the test_latest_bedrock_model_names_geo_prefixes_are_supported function to remove the file path reference, enhancing clarity.
|
@dsfaccini Thanks so much for taking a look! I've addressed your comments here. |
|
@Leundai thanks again for the changes! Code looks great now, I think after the two requested changes we'll be good to go. |
This reduces the lines of code and also adjusts our tuple to now not have the period at the end. This will simplify our code.
|
Great! Thanks for taking another look here, simplified this even further |
| def test_bedrock_provider_model_profile_with_unknown_geo_prefix(env: TestEnv): | ||
| env.set('AWS_DEFAULT_REGION', 'us-east-1') | ||
| provider = BedrockProvider() | ||
|
|
||
| model_name = 'narnia.anthropic.claude-sonnet-4-5-20250929-v1:0' | ||
| profile = provider.model_profile(model_name) | ||
| assert profile is None, f'model_profile returned {profile} for {model_name}' |
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.
@DouweM this is in line with previous behavior but probably bad UX, right? should we parse the anthropic provider properly even if we haven't registered a potential new region prefix? and print a warning?
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.
i don't know if we should properly parse if it's an incorrect geo prefix, as a user id be confused that a non existent geo prefix actually worked.
might be worth printing a warning and returning None still? or just raising an exception (unsure if that's a huge nono)
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.
I think this behavior is fine, you'd end up getting an error from the API anyway
us-gov. and other multi-character Bedrock geo prefixes
Summary
The
BedrockProvider.model_profilemethod previously only handled 2-character regional prefixes (e.g.,us.,eu.), causing issues with models using longer prefixes likeus-gov.(AWS GovCloud) andglobal..Problem
When using extended thinking with
us-gov.anthropic.*models via Bedrock, themodel_profilemethod doesn't recognize theus-gov.prefix because it only strips 2-character prefixes:This causes
bedrock_send_back_thinking_partsto stay at its defaultFalsevalue. As a result,ThinkingPartblocks from previous conversation turns are converted to text blocks with thinking tags instead ofreasoningContent, causing Bedrock to reject the request with:Solution
_strip_geo_prefix()helper function that properly handles all known geo prefixes includingus-gov.andglobal._AWS_BEDROCK_INFERENCE_GEO_PREFIXESconstant to includeus-gov.(the comment in the code already suggested this)Affected Models
us-gov.anthropic.claude-sonnet-4-5-20250929-v1:0us-gov.anthropic.claude-3-7-sonnet-20250219-v1:0global.anthropic.claude-opus-4-5-20251101-v1:0Testing
All existing tests pass, plus new parametrized tests for all geo prefixes