feat(bedrock): Fetch model list from Bedrock#4834
Conversation
…d list Signed-off-by: Jeremy McAnally <jmcanally@sequoiacap.com>
…etting the full list of usable model IDs and profile ARNs
|
Ah whoops just seeing the failures on this. Builds clean for me, but I'll get this fixed up early next week. 👍 |
|
thank you. sorry for the delay in replying. I started looking at this and then saw that bedrock can hang the entire app because of some dumb not async thing. I made main async for provider init. might help here too. shout if you need a review |
|
do we still want to get this in? |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for dynamically fetching available Bedrock models and updates the default model configuration. The main changes enhance the Bedrock provider to query AWS APIs for available models and inference profiles rather than relying solely on a static list.
- Adds
fetch_supported_modelsimplementation to query AWS Bedrock APIs - Updates default model to a globally-available variant and refreshes the known models list
- Adds new dependency
aws-sdk-bedrockfor control plane operations
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| crates/goose/Cargo.toml | Adds aws-sdk-bedrock dependency for control plane API access |
| Cargo.lock | Updates lock file with new aws-sdk-bedrock package and transitive dependency version changes |
| crates/goose/src/providers/bedrock.rs | Implements dynamic model fetching via AWS APIs, updates default model, and adds helper methods for querying inference profiles and foundation models |
| ui/desktop/src/components/settings/extensions/deeplink.ts | Formats allowed commands array for better readability (multi-line) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client, | ||
| model, | ||
| retry_config, | ||
| sdk_config |
There was a problem hiding this comment.
Missing trailing comma after sdk_config in struct instantiation. This is inconsistent with Rust style conventions and makes future diffs larger when adding new fields.
| sdk_config | |
| sdk_config, |
| Ok(profiles) | ||
| } | ||
| Err(err) => { | ||
| return Err(err) |
There was a problem hiding this comment.
The explicit return statement is unnecessary here. Simply use Err(err) without return, which is more idiomatic Rust.
| return Err(err) | |
| Err(err) |
| Ok(models) | ||
| } | ||
| Err(err) => { | ||
| return Err(err) |
There was a problem hiding this comment.
The explicit return statement is unnecessary here. Simply use Err(err) without return, which is more idiomatic Rust.
| return Err(err) | |
| Err(err) |
| let mut application_profiles = match self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::Application).await { | ||
| Ok(profiles) => profiles, | ||
| Err(profile_error) => return Err(profile_error) | ||
| }; | ||
|
|
||
| let mut system_profiles = match self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::SystemDefined).await { | ||
| Ok(profiles) => profiles, | ||
| Err(profile_error) => return Err(profile_error) | ||
| }; | ||
|
|
||
| let mut foundation_models = match self.fetch_foundation_models().await { | ||
| Ok(models) => models, | ||
| Err(model_error) => return Err(model_error) | ||
| }; |
There was a problem hiding this comment.
Consider using the ? operator instead of explicit match for error propagation. Replace with let mut application_profiles = self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::Application).await?; for more concise and idiomatic code.
| let mut application_profiles = match self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::Application).await { | |
| Ok(profiles) => profiles, | |
| Err(profile_error) => return Err(profile_error) | |
| }; | |
| let mut system_profiles = match self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::SystemDefined).await { | |
| Ok(profiles) => profiles, | |
| Err(profile_error) => return Err(profile_error) | |
| }; | |
| let mut foundation_models = match self.fetch_foundation_models().await { | |
| Ok(models) => models, | |
| Err(model_error) => return Err(model_error) | |
| }; | |
| let mut application_profiles = self | |
| .fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::Application) | |
| .await?; | |
| let mut system_profiles = self | |
| .fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::SystemDefined) | |
| .await?; | |
| let mut foundation_models = self.fetch_foundation_models().await?; |
| let mut system_profiles = match self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::SystemDefined).await { | ||
| Ok(profiles) => profiles, | ||
| Err(profile_error) => return Err(profile_error) | ||
| }; |
There was a problem hiding this comment.
Consider using the ? operator instead of explicit match for error propagation. Replace with let mut system_profiles = self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::SystemDefined).await?; for more concise and idiomatic code.
| let mut system_profiles = match self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::SystemDefined).await { | |
| Ok(profiles) => profiles, | |
| Err(profile_error) => return Err(profile_error) | |
| }; | |
| let mut system_profiles = self.fetch_inference_profiles(aws_sdk_bedrock::types::InferenceProfileType::SystemDefined).await?; |
| let mut foundation_models = match self.fetch_foundation_models().await { | ||
| Ok(models) => models, | ||
| Err(model_error) => return Err(model_error) | ||
| }; |
There was a problem hiding this comment.
Consider using the ? operator instead of explicit match for error propagation. Replace with let mut foundation_models = self.fetch_foundation_models().await?; for more concise and idiomatic code.
| let mut foundation_models = match self.fetch_foundation_models().await { | |
| Ok(models) => models, | |
| Err(model_error) => return Err(model_error) | |
| }; | |
| let mut foundation_models = self.fetch_foundation_models().await?; |
| // the list of available on demand models since | ||
| // they are user/account created and more likely | ||
| // to be what they want to use. | ||
| application_profiles.sort_unstable(); |
There was a problem hiding this comment.
Trailing whitespace detected at the end of the line. Remove the extra spaces after the semicolon.
| application_profiles.sort_unstable(); | |
| application_profiles.sort_unstable(); |
| let response = bedrock_control_client | ||
| .list_inference_profiles() | ||
| .max_results(BEDROCK_MAX_MODEL_RESULTS) | ||
| .type_equals(profile_type.clone()) |
There was a problem hiding this comment.
The profile_type.clone() is unnecessary here. The type_equals method likely accepts a reference. Consider passing &profile_type or profile_type directly if it implements Copy to avoid the clone operation.
| .type_equals(profile_type.clone()) | |
| .type_equals(profile_type) |
|
Thanks for the work on this. Going to close this out per #4834 (comment) being three weeks ago, but welcome to re-open a new PR. |
3 participants
Summary
This PR implements
fetch_supported_modelsfor the AWS Bedrock provider. Currently, the Bedrock provider is unusable out of the box unless you supply a custom model ID since all of the entries in the currentBEDROCK_KNOWN_MODELSarray aren't usable as model IDs for invocations. This solves that issue as well as implementing the helper to grab the list of usable model IDs/ARNs from Bedrock directly.Changes
aws-sdk-bedrockcrate, not to be confused with the currently includedaws-sdk-bedrockruntimecrate. This crate is required for getting information about available models/inference profiles whereas the other crate is only for invoking models and agents.fetch_supported_modelsby fetching the user's configured application inference profiles and then the models that are available for on-demand inference. The inference profiles are listed first (sorted) followed by the on-demand list (sorted). From a UX perspective, that seems like the most useful sorting to me since I'm probably wanting to use inference profiles I've configured, but obviously I'm not stuck to that for any real reason.BEDROCK_KNOWN_MODELSlist with a little more variety in providers. If the user's account hasn't enabled any of the models in Bedrock, this list isn't that useful1, but at least if they only have invoke privileges (i.e., rather than list/get/admin privileges) on their IAM role, they can use the most popular ones without needing to input something custom. With the current list in the code, even if the model selected from the list is enabled it won't work.(Side note: A similar PR is incoming for Azure OpenAI...)
Footnotes
That issue will mostly go away in October 2025 when they will start enabling all models by default and access will be controlled solely by the user's IAM policy. ↩