fix: change AMI ids in tests to be dynamic based on regions#1004
Conversation
…to Absolute or normalized path
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
|
||
| echo "Mounting EFS File Systems" | ||
| sudo yum install -y amazon-efs-utils.noarch 0:1.10-1.amzn2 | ||
| sudo yum install -y amazon-efs-utils |
There was a problem hiding this comment.
do different regions have different versions released?
There was a problem hiding this comment.
The list for the AMI ids are Amazon Linux AMI 2018, while the old hard-coded one is Amazon Linux2 AMI.
The new list needs amazon-efs-utils.noarch 0:1.10-1.amzn1 version. I just don't want specify specific version in case some of them need amazon-efs-utils.noarch 0:1.10-1.amzn2 version.
But it will automatically select the correct version if i don't specify
| ROLE_NAME = "SageMakerRole" | ||
| REGION = "us-west-2" | ||
| EC2_INSTANCE_TYPE = "t2.micro" | ||
| AMI_ID = "ami-082b5a644766e0e6f" |
There was a problem hiding this comment.
I think you can remove this
There was a problem hiding this comment.
Forgot to remove it
| KEY_PATH = os.path.join(tempfile.gettempdir(), FILE_NAME) | ||
| STORAGE_CAPACITY_IN_BYTES = 3600 | ||
|
|
||
| AWSRegionArch2AMI = { |
There was a problem hiding this comment.
rename this to REGION_TO_AMI_MAP - we still want to honor Python capitalization/naming convention
| "cn-north-1": "ami-0a4eaf6c4454eda75", | ||
| "cn-northwest-1": "ami-6b6a7d09", | ||
| "us-gov-west-1": "ami-906cf0f1", | ||
| } |
There was a problem hiding this comment.
@caxiaohu @laurenyu Is there any way we could make this dynamic based on the AMI name or type? Since we only need a basic AL AMI?
i.e.: Some dynamic logic that searches for an AMI by name and uses whatever ID is appropriate in that region. Eliminating the need for a map that we need to maintain.
I'm concerned that we'll need to add to this list for all future regions. This is likely another time-bomb that'll trigger a page on region expansion (or the dev will catch it during region expansion).
There was a problem hiding this comment.
good catch. I'd forgotten that I found this yesterday; I think it should do what we want: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/finding-an-ami.html#finding-quick-start-ami
There was a problem hiding this comment.
The AMI ids search results:
| Region | AMI id |
|---|---|
| us-west-1 | ami-0fcdcdb074d2bac5f |
| us-west-2 | ami-0f2176987ee50226e |
| us-east-1 | ami-035b3c7efe6d061d5 |
| us-east-2 | ami-02f706d959cedf892 |
| eu-west-1 | ami-0862aabda3fb488b5 |
| eu-west-2 | ami-0bdfa1adc3878cd23 |
| eu-west-3 | ami-05b93cd5a1b552734 |
| eu-central-1 | ami-026d3b3672c6e7b66 |
| ap-northeast-1 | ami-04b2d1589ab1d972c |
| ap-northeast-2 | ami-0be3e6f84d3b968cd |
| ap-northeast-3 | ami-0639e0e4c18187b30 |
| ap-southeast-1 | ami-0fb6b6f9e81056553 |
| ap-southeast-2 | ami-075caa3491def750b |
| ap-south-1 | ami-0b99c7725b9484f9e |
| ca-central-1 | ami-0a67d15f2858e33cb |
| sa-east-1 | ami-0bb96001cf2299257 |
| cn-north-1 | AWS was not able to validate the provided access credentials |
| cn-northwest-1 | AWS was not able to validate the provided access credentials |
| us-gov-west-1 | AWS was not able to validate the provided access credentials |
All of them are Amazon Linux AMI 2018.03.0
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| return fs_resources | ||
|
|
||
|
|
||
| def _dynamic_ami_id(sagemaker_session): |
There was a problem hiding this comment.
let's rename this to _ami_id_for_region
| def _dynamic_ami_id(sagemaker_session): | ||
| ec2_client = sagemaker_session.boto_session.client("ec2") | ||
| filters = [ | ||
| {"Name": "name", "Values": ["amzn-ami-hvm-????.??.?.????????-x86_64-gp2"]}, |
There was a problem hiding this comment.
let's make this a constant. that'll both make it easier to find if it needs to be changed later for whatever reason and help describe what the string is.
| image_details = sorted(response["Images"], key=itemgetter("CreationDate"), reverse=True) | ||
| if len(image_details) > 0: | ||
| ami_id = image_details[0]["ImageId"] | ||
| return ami_id |
There was a problem hiding this comment.
nit: you can combine ll. 126-127
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| STORAGE_CAPACITY_IN_BYTES = 3600 | ||
|
|
||
| AMI_FILTERS = [ | ||
| {"Name": "name", "Values": ["amzn-ami-hvm-????.??.?.????????-x86_64-gp2"]}, |
There was a problem hiding this comment.
sorry for the confusion. I meant specifically "amzn-ami-hvm-????.??.?.????????-x86_64-gp2" - it's not really obvious what this string means
There was a problem hiding this comment.
I see what you mean now
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
* feature: method to build pipeline parameters from existing execution … (#951) * feature: method to build pipeline parameters from existing execution with optional value overrides * fix style check * assert error message in unit test * feature: allow opt out from referencing latest execution in the selec… (#1004) * fix: Update pipeline.py and selective_execution_config.py with small fixes (#1099) --------- Co-authored-by: stacicho <[email protected]> Co-authored-by: Zuoyuan Huang <[email protected]>
Issue #, if available:
Description of changes:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.