-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: 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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -58,6 +58,23 @@ def bedrock_deepseek_model_profile(model_name: str) -> ModelProfile | None: | |
| return profile # pragma: no cover | ||
|
|
||
|
|
||
| # Known geo prefixes for cross-region inference profile IDs | ||
| _BEDROCK_GEO_PREFIXES: tuple[str, ...] = ('us.', 'eu.', 'apac.', 'jp.', 'au.', 'ca.', 'global.', 'us-gov.') | ||
|
|
||
|
|
||
| def _strip_geo_prefix(model_name: str) -> str: | ||
| """Strip geographic/regional prefix from model name if present. | ||
|
|
||
| AWS Bedrock cross-region inference uses prefixes like 'us.', 'eu.', 'us-gov.', 'global.' | ||
| to route requests to specific regions. This function strips those prefixes so we can | ||
| identify the underlying provider and model. | ||
| """ | ||
| for prefix in _BEDROCK_GEO_PREFIXES: | ||
| if model_name.startswith(prefix): | ||
| return model_name.removeprefix(prefix) | ||
| return model_name | ||
|
|
||
|
|
||
| class BedrockProvider(Provider[BaseClient]): | ||
| """Provider for AWS Bedrock.""" | ||
|
|
||
|
|
@@ -87,13 +104,11 @@ def model_profile(self, model_name: str) -> ModelProfile | None: | |
| 'deepseek': bedrock_deepseek_model_profile, | ||
| } | ||
|
|
||
| # Split the model name into parts | ||
| parts = model_name.split('.', 2) | ||
|
|
||
| # Handle regional prefixes (e.g. "us.") | ||
| if len(parts) > 2 and len(parts[0]) == 2: | ||
| parts = parts[1:] | ||
| # Strip regional/geo prefix if present (e.g. "us.", "eu.", "us-gov.", "global.") | ||
|
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. you can remove this comment |
||
| model_name = _strip_geo_prefix(model_name) | ||
|
|
||
| # Split the model name into provider and model parts | ||
| parts = model_name.split('.', 1) | ||
| if len(parts) < 2: | ||
| return None | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,3 +100,50 @@ def test_bedrock_provider_model_profile(env: TestEnv, mocker: MockerFixture): | |
|
|
||
| unknown_model = provider.model_profile('unknown.unknown-model') | ||
| assert unknown_model is None | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('prefix', ['us.', 'eu.', 'apac.', 'jp.', 'au.', 'ca.', 'global.', 'us-gov.']) | ||
|
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. can we use the |
||
| def test_bedrock_provider_model_profile_all_geo_prefixes(env: TestEnv, prefix: str): | ||
| """Test that all cross-region inference geo prefixes are correctly handled. | ||
|
|
||
| This is critical for AWS GovCloud (us-gov.) and other regional deployments | ||
| where models use prefixes longer than 2 characters. | ||
| """ | ||
| env.set('AWS_DEFAULT_REGION', 'us-east-1') | ||
| provider = BedrockProvider() | ||
|
|
||
| # Test Anthropic model with geo prefix | ||
| model_name = f'{prefix}anthropic.claude-sonnet-4-5-20250929-v1:0' | ||
| profile = provider.model_profile(model_name) | ||
|
|
||
| assert profile is not None, f'model_profile returned None for {model_name}' | ||
| assert isinstance(profile, BedrockModelProfile) | ||
| assert profile.bedrock_supports_tool_choice is True | ||
| assert profile.bedrock_send_back_thinking_parts is True | ||
|
Comment on lines
+120
to
+122
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. if these don't vary across models we don't need to test them, right? the test just tests that all prefixes are hadnled properly |
||
|
|
||
|
|
||
| def test_bedrock_provider_model_profile_us_gov_anthropic(env: TestEnv, mocker: MockerFixture): | ||
| """Test that us-gov. prefixed Anthropic models get the correct profile. | ||
|
|
||
| This specifically tests the us-gov. prefix which was previously broken | ||
| because the provider only handled 2-character prefixes. | ||
| """ | ||
| env.set('AWS_DEFAULT_REGION', 'us-east-1') | ||
| provider = BedrockProvider() | ||
|
|
||
| ns = 'pydantic_ai.providers.bedrock' | ||
| anthropic_model_profile_mock = mocker.patch(f'{ns}.anthropic_model_profile', wraps=anthropic_model_profile) | ||
|
|
||
| # Test us-gov. prefix (AWS GovCloud cross-region inference) | ||
| profile = provider.model_profile('us-gov.anthropic.claude-sonnet-4-5-20250929-v1:0') | ||
| anthropic_model_profile_mock.assert_called_with('claude-sonnet-4-5-20250929') | ||
| assert isinstance(profile, BedrockModelProfile) | ||
| assert profile.bedrock_supports_tool_choice is True | ||
| assert profile.bedrock_send_back_thinking_parts is True | ||
|
|
||
| # Test global. prefix | ||
| profile = provider.model_profile('global.anthropic.claude-opus-4-5-20251101-v1:0') | ||
| anthropic_model_profile_mock.assert_called_with('claude-opus-4-5-20251101') | ||
| assert isinstance(profile, BedrockModelProfile) | ||
| assert profile.bedrock_supports_tool_choice is True | ||
| assert profile.bedrock_send_back_thinking_parts is True | ||
|
Comment on lines
+125
to
+149
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. we're already testing this above, aren't we? let's add a test to check that we don't have any bedrock shorthand in |
||
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 at some point it might be worth unifying this and the other private constant in
models/bedrock.pyBut this just extends it to support govcloud and global. Its been a real issue since Claude 4.5 came out and AWS doing cross-region inferencing
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 you should definitely not duplicate it, since we're already importing the
BedrockModelProfileinmodels/bedrock.pywe should make this public and import it there.pydantic-ai/pydantic_ai_slim/pydantic_ai/models/bedrock.py
Line 46 in 8d111fe
@DouweM do you agree?