Stop reimplementing region resolution; let the boto3 session do it - #29
Merged
Conversation
art-dsit
force-pushed
the
bundle-provisioned-instance
branch
from
July 8, 2026 15:58
7e61f51 to
fe39de8
Compare
art-dsit
force-pushed
the
region-handling-cleanup
branch
from
July 9, 2026 08:29
ca444c6 to
0d89f7b
Compare
Region was resolved by a hand-rolled os.getenv("AWS_REGION",
"AWS_DEFAULT_REGION") ladder in ~two places. That reimplements botocore's
own chain incompletely (it skips the active profile's ~/.aws/config), and
schema.py turned "no region configured" into a ValueError instead of the
loud NoRegionError boto3 raises.
Let the session resolve region: build the client with
region_name=config.region (an explicit override, or None to fall through
the chain) and read the concrete value back off client.meta.region_name
for the AMI lookup, SSM client, and ProvisionedInstance/SandboxInstanceInfo.
region stays instance data.
- schema.from_settings: drop the AWS_REGION ladder + the required-region
ValueError; region is now an optional override (None -> session resolves).
- DefaultEc2InstanceProvider: region no longer a required config field;
resolve it off the ec2 client in create_instance and find_sandbox_instances.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that region is resolved from the boto3 chain rather than a required config field, an eval that hardcodes ami_id (the README's recommended "portable eval" shape) but is silent on region will pick up the runner's AWS_DEFAULT_REGION. If that differs from the AMI's region, run_instances fails with a raw InvalidAMIID.NotFound — opaque to someone running an eval they didn't write. Translate the AMI-not-found ClientError (and the empty describe_images result on the volume_size path) into a ValueError naming the AMI, the resolved region, and the fix (set INSPECT_EC2_SANDBOX_REGION, or omit ami_id to auto-resolve for the region). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
art-dsit
force-pushed
the
region-handling-cleanup
branch
from
July 9, 2026 09:01
0d89f7b to
dadf275
Compare
Both spots explained an absence rather than adding information beyond what the surrounding code already shows.
Each call site already explains the resolve-then-read-back pattern inline; the docstring version just repeated it while emphasizing what used to be required.
art-dsit
marked this pull request as ready for review
July 9, 2026 09:21
The README and the config comment listed the resolution chain as "AWS_REGION / AWS_DEFAULT_REGION / ~/.aws/config", but boto3 does not read AWS_REGION — botocore binds the region config variable to AWS_DEFAULT_REGION only (unlike the AWS CLI and the JS/Go/Java SDKs). A user setting only AWS_REGION gets a NoRegionError, not the region they expect. Signpost boto3's own configuration guide for the chain rather than duplicating it, and state the AWS_REGION gotcha once, prominently, in the README. Drop the enumerated chain from the schema comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The AMI/region-mismatch translation lived only on the run_instances path. When volume_size is set, create_instance first calls describe_images (via _root_device_name) to find the root device, and a foreign-region AMI makes describe_images *raise* InvalidAMIID.NotFound rather than return an empty list — so the raw botocore ClientError leaked with a traceback instead of the region-scoped hint. (The old comment claimed describe_images returns [] for a foreign-region AMI; that's actually the private/deregistered case.) Wrap describe_images with the same _AMI_NOT_FOUND_CODES translation and keep the empty-list branch for the can't-see-this-AMI case. Verified e2e against a real eu-west-1 AMI requested in eu-west-2 with volume_size set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
||
| # Optional explicit region override. None -> the boto3 session resolves the | ||
| # region when it builds a client (see README); a set value overrides that. | ||
| region: Optional[str] = None |
There was a problem hiding this comment.
nit: i think the Optional[str] syntax is out-of-date and str | None is favoured (although I can see this line is just moved from below)
Collaborator
Author
There was a problem hiding this comment.
I'll leave it for now as it would need changing everywhere, but maybe do as a later refactor
There was a problem hiding this comment.
nit: the comment here is no longer accurate
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Region was resolved by a hand-rolled
os.getenv("AWS_REGION", os.getenv("AWS_DEFAULT_REGION", ...))ladder in two places. It reimplements botocore's own resolution chain incompletely — it skips the active profile's~/.aws/config— andschema.from_settingsturned "no region configured" into aValueErrorinstead of the loudNoRegionErrorboto3 raises.Fix: let the session resolve region. Build the client with
region_name=config.region(an explicit override, orNoneto fall through the standard chain) and read the concrete value back offclient.meta.region_namefor the AMI lookup, the SSM client, andProvisionedInstance/SandboxInstanceInfo.regionstays instance data (kept on both dataclasses).schema.from_settings: drop theAWS_REGIONladder and the required-regionValueError;regionis now an optional override (None→ session resolves).DefaultEc2InstanceProvider:regionis no longer a required config field; resolved off the ec2 client increate_instanceandfind_sandbox_instances(the latter's ladder was added by Bundle ProvisionedInstance through the sandbox env #16 and is now unnecessary).Behavioural change (intentional)
boto3 reads the region from
AWS_DEFAULT_REGIONand~/.aws/config, notAWS_REGION— unlike the AWS CLI and the JS/Go/Java SDKs. So anyone who was relying onAWS_REGIONalone now gets aNoRegionErrorinstead of a silently-defaulted region. This is the point of the change (fail loud, not wrong), and it's called out in the README and CHANGELOG. SetAWS_DEFAULT_REGIONorINSPECT_EC2_SANDBOX_REGION.Why AMI-error handling is in a "region" PR
The title is about region resolution, but the diff also translates
InvalidAMIID.*(and empty-describe_images) into a clearValueErrorwith a fix hint. That's deliberate: a hardcoded AMI from another region is exactly the silent failure the old"eu-west-2"-style default masked — once region comes from the session, an eval that pinsami_idonly runs in that AMI's region, and the raw boto error (InvalidAMIID.NotFound) doesn't say why. Covered by four new tests (both theClientErrorand empty-Imagespaths, run-instances and describe-images).