-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
chore(ec2): dl2q instance type, IntanceType
fixes
#29481
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple questions.
* High performance computing powered by AWS Trainium | ||
*/ | ||
TRN1 = 'trn1', | ||
TRAINING_ACCELERATOR1 = 'training-accelerator1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these descriptions pulled from? Some of them seem mismatched after the changes, or were they incorrect before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither trn1
nor trn1n
had a symbolic "alias" given to them when they were added, unlike every other instance class. I added one, didn't get it from anywhere really, looked up the AWS docs and tried to match the other's.
* Network-optimized high performance computing powered by AWS Trainium | ||
*/ | ||
TRN1N = 'trn1n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description for this has changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the trn1n
description didn't have the network optimization discriminator, both trn1*
had the same description:
aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/instance-types.ts
Lines 614 to 622 in fac4a9c
/** | |
* High performance computing powered by AWS Trainium | |
*/ | |
TRN1 = 'trn1', | |
/** | |
* High performance computing powered by AWS Trainium | |
*/ | |
TRN1N = 'trn1n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks for clarifying. Waiting for one other PR to merge in and will approve this after that.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Issue # (if applicable)
None
Reason for this change
Follow up to #29427. Added a missing instance type, and minor fixes to the
IntanceType
classDescription of changes
I'd missed an instance type,
dl2q
, which was neither inus-east-1
orus-east-2
, but inus-west-2
.I've also added a couple of missing symbolic names, as well as fixed some differing comments between the key and its symbolic value (e.g.
M3
andSTANDARD3
)I also re-ordered a couple of enum values, when the symbolic value was separated from its match
Description of how you validated changes
Compared the CDK to the SDK to find the missing instance. Programmatically iterated over the comments of
IntanceType
to make sure the comments of the symbolic keys matched the one below.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license