Skip to content

feat: update AWS Bedrock cross-region inference profile mapping, Closed issue #2704 #4973

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+33 −11
Open

feat: update AWS Bedrock cross-region inference profile mapping, Closed issue #2704 #4973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/types/src/providers/bedrock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,24 @@ export const BEDROCK_MAX_TOKENS = 4096

export const BEDROCK_DEFAULT_CONTEXT = 128_000

// AWS Bedrock Inference Profile mapping based on official documentation
// https://docs.aws.amazon.com/bedrock/latest/userguide/inference-profiles-support.html
// This mapping prioritizes AWS's recommended inference profiles for optimal cross-region support
export const AWS_INFERENCE_PROFILE_MAPPING: Record<string, string> = {
// Americas regions → us. inference profile
"us-": "us.",
// Europe regions → eu. inference profile
"eu-": "eu.",
// Asia Pacific regions → apac. inference profile (updated per AWS documentation)
"ap-": "apac.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I notice the comment mentions "apac." as the updated prefix per AWS documentation, which is great. However, the existing BEDROCK_REGION_INFO object below (lines 422-427) still contains the old "ap." prefix entry. Is this intentional for backward compatibility? If so, it might be worth adding a note to clarify the relationship between the old and new prefixes.

// Canada regions → ca. inference profile
"ca-": "ca.",
// South America regions → sa. inference profile
"sa-": "sa.",
// US Government Cloud → ug. inference profile
"us-gov-": "ug.",
}

export const BEDROCK_REGION_INFO: Record<
string,
{
Expand Down
26 changes: 15 additions & 11 deletions src/api/providers/bedrock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
BEDROCK_MAX_TOKENS,
BEDROCK_DEFAULT_CONTEXT,
BEDROCK_REGION_INFO,
AWS_INFERENCE_PROFILE_MAPPING,
} from "@roo-code/types"

import { ApiStream } from "../transform/stream"
Expand Down Expand Up @@ -900,14 +901,12 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
//a model was selected from the drop down
modelConfig = this.getModelById(this.options.apiModelId as string)

if (this.options.awsUseCrossRegionInference) {
// Get the current region
const region = this.options.awsRegion || ""
// Use the helper method to get the appropriate prefix for this region
const prefix = AwsBedrockHandler.getPrefixForRegion(region)

// Apply the prefix if one was found, otherwise use the model ID as is
modelConfig.id = prefix ? `${prefix}${modelConfig.id}` : modelConfig.id
// Add cross-region inference prefix if enabled
if (this.options.awsUseCrossRegionInference && this.options.awsRegion) {
const prefix = AwsBedrockHandler.getPrefixForRegion(this.options.awsRegion)
if (prefix) {
modelConfig.id = `${prefix}${modelConfig.id}`
}
}
}

Expand Down Expand Up @@ -978,11 +977,16 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
}

private static getPrefixForRegion(region: string): string | undefined {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be helpful to add test coverage for this new getPrefixForRegion method? I noticed there are existing tests for cross-region inference in bedrock-custom-arn.spec.ts, but they don't specifically test the new mapping logic. Some test cases to consider:

  • Testing that ap-northeast-1 returns "apac."
  • Testing that us-gov-west-1 returns "ug." (not "us.")
  • Testing that an unknown region pattern returns undefined

This would help ensure the mapping works correctly and prevent regressions.

for (const [prefix, info] of Object.entries(BEDROCK_REGION_INFO)) {
if (info.pattern && region.startsWith(info.pattern)) {
return prefix
// Use AWS recommended inference profile prefixes
// Sort by prefix length (descending) to ensure more specific prefixes match first (e.g., us-gov- before us-)
const sortedMappings = Object.entries(AWS_INFERENCE_PROFILE_MAPPING).sort(([a], [b]) => b.length - a.length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add a comment explaining this? It would help future maintainers understand why the sorting is necessary:

// Sort by prefix length (descending) to ensure more specific prefixes match first
// This prevents "us-" from matching "us-gov-" regions, for example
const sortedMappings = Object.entries(AWS_INFERENCE_PROFILE_MAPPING).sort(([a], [b]) => b.length - a.length)


for (const [regionPattern, inferenceProfile] of sortedMappings) {
if (region.startsWith(regionPattern)) {
return inferenceProfile
}
}

return undefined
}

Expand Down