Use AWS_DEFAULT_REGION for bedrock if provided #157
Conversation
src/autogluon/assistant/llm/llm.py
Outdated
| @@ -115,8 +115,7 @@ def get_openai_models() -> List[str]: | |||
| @staticmethod | |||
| def get_bedrock_models() -> List[str]: | |||
| try: | |||
| # TODO: Remove hardcoding AWS region | |||
| bedrock = boto3.client("bedrock", region_name="us-west-2") | |||
| bedrock = boto3.client("bedrock", region_name=os.environ.get("AWS_DEFAULT_REGION", "us-west-2")) | |||
There was a problem hiding this comment.
If you leave out region_name in the client creation, boto3 handles this fallback behavior automatically. This might be cleaner where we don't need to take care of passing the region everytime a boto3 client initiated in codes
There was a problem hiding this comment.
I agree we could simply remove the region_name kwarg, the only thing being added here is that we have a fallback in case the user doesn't set AWS_DEFAULT_REGION. But I'm happy to remove this as well.
There was a problem hiding this comment.
The default behavior uses AWS_DEFAULT_REGION or the AWS config file, and as a last resort, it resorts to IMDS for EC2. If none of these sources provide a region resolution, a NoRegionError is raised to ensure that the user has to explicitly set the region in the running environment. I believe this approach is better than selecting a region on behalf of the user.
There was a problem hiding this comment.
cleaned it up
| @@ -29,8 +29,7 @@ def __init__( | |||
| num_iterations: int = 2, | |||
| optimization_metric: str = "roc", | |||
| eval_model: str = "lightgbm", | |||
| # TODO: Remove hardcoding AWS region | |||
| region_name: str = "us-west-2", | |||
| region_name: str = os.environ.get("AWS_DEFAULT_REGION", "us-west-2"), | |||
There was a problem hiding this comment.
@tonyhoo it is still required to pass the region_name here as for CAAFE, so I think it is okay to keep this.
Later after the release I'm going to refactor CAAFE, and then this argument can also be removed.
There was a problem hiding this comment.
I think it would be better to keep the region setting consistent across the repo
There was a problem hiding this comment.
It's not possible to remove it from CAAFE until we make changes in CAAFE upstream. Would you suggest that I also make changes upstream? See details here:
Description
How Has This Been Tested?
pytest tests/)aga run knot_theory --config_overrides "time_limit=120, feature_transformers.enabled_models=[CAAFE]"Configuration Changes
Type of Change