-
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
feat(stepfunctions-tasks): FastFile mode for SageMaker Training Job #26675
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.
Looks good overall.
I think that since we are here we should add validation for the algorithmName
parameter in the SageMakerCreateTrainingJob
constructor following this spec, and the related unit tests.
Thank you for the review. |
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts
Outdated
Show resolved
Hide resolved
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.
Thanks for the follow-up work 💪
I left you a couple of comments on things that I think should be fixed
…ws-cdk into feature/stepfunctions/sagemaker
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/base-types.ts
Outdated
Show resolved
Hide resolved
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.
Looks great, thank you 👍
I left you a comment on the enum value name (sorry, I missed that in the previous review).
If you could fix that then this will be good to go for me.
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
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.
Hi @tmyoda! Thanks for this PR. Here is my additional feedback:
- there looks to be two things here, fastFile mode and validation for algorithm names. they belong in separate PRs
- fastFile mode is a feature, not a fix, which also means that I would like to see an update to the README section detailing how to use fast file (and what it is haha).
@@ -324,6 +332,21 @@ export class SageMakerCreateTrainingJob extends sfn.TaskStateBase implements iam | |||
: {}; | |||
} | |||
|
|||
private validateAlgorithmName(algorithmName?: string): void { |
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.
this doesn't have anything to do with FastFile mode, right? Can it be included in a separate PR?
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.
Alright, will create another PR for the algorithm name validation
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-training-job.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-training-job.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-training-job.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-training-job.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-training-job.test.ts
Outdated
Show resolved
Hide resolved
…ws-cdk into feature/stepfunctions/sagemaker
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
I plan to create another PR for algorithm name validation once it's merged |
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). |
…reateTrainingJob` (#26877) Referencing PR #26675, I have added validation for the `algorithmName` parameter in `SageMakerCreateTrainingJob`. However, it was suggested that changes for validation should be separated. So, I have created this PR. Docs for `algorithmName`: https://docs.aws.amazon.com/sagemaker/latest/APIReference/API_AlgorithmSpecification.html#API_AlgorithmSpecification_Contents Exemption Request: This change does not alter the behavior. I believe the unit test `create-training-job.test.ts` that I have added is sufficient to test this change. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
According to the AWS documentation, the TrainingInputMode for a SageMaker Training Job can be one of the following:
Pipe | File | FastFile
https://docs.aws.amazon.com/sagemaker/latest/APIReference/API_Channel.html#sagemaker-Type-Channel-InputMode
https://docs.aws.amazon.com/sagemaker/latest/APIReference/API_AlgorithmSpecification.html#API_AlgorithmSpecification_Contents
I have just added
FastFile
below to align with the official documentation.https://github.com/aws/aws-cdk/blob/v2.90.0/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/base-types.ts#L458
Closes #26653.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license