feat(stepfunctions-tasks): FastFile mode for SageMaker Training Job by tmyoda · Pull Request #26675 · aws/aws-cdk

tmyoda · 2023-08-09T00:11:05Z

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

lpizzinidev

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.

tmyoda · 2023-08-09T23:42:44Z

Thank you for the review.
I will add validation for algorithmName in this PR.

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts

lpizzinidev

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

lpizzinidev

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.

aws-cdk-automation

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.

kaizencc

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).

kaizencc · 2023-08-16T19:57:23Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts

      : {};
  }

+  private validateAlgorithmName(algorithmName?: string): void {


this doesn't have anything to do with FastFile mode, right? Can it be included in a separate PR?

Alright, will create another PR for the algorithm name validation

packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-training-job.test.ts

…ws-cdk into feature/stepfunctions/sagemaker

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

tmyoda · 2023-08-19T19:39:15Z

I plan to create another PR for algorithm name validation once it's merged

mergify · 2023-08-23T16:11:45Z

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-cdk-automation · 2023-08-23T21:48:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: baf02cb
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-08-23T21:48:38Z

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*

fix(stepfunctions-tasks): FastFile mode for SageMaker Training Job

2e49897

aws-cdk-automation requested a review from a team August 9, 2023 00:11

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Aug 9, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 9, 2023

lpizzinidev suggested changes Aug 9, 2023

View reviewed changes

tmyoda and others added 2 commits August 12, 2023 01:02

add: validation for algorithmName

03c77ec

Merge branch 'main' into feature/stepfunctions/sagemaker

bcfae4c

tmyoda requested a review from lpizzinidev August 12, 2023 02:19

lpizzinidev reviewed Aug 12, 2023

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts Outdated Show resolved Hide resolved

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 12, 2023

lpizzinidev reviewed Aug 12, 2023

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts Outdated Show resolved Hide resolved

lpizzinidev reviewed Aug 12, 2023

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts Outdated Show resolved Hide resolved

lpizzinidev suggested changes Aug 12, 2023

View reviewed changes

tmyoda and others added 3 commits August 12, 2023 21:49

add validation

65b304f

Merge branch 'feature/stepfunctions/sagemaker' of github.com:tmyoda/a…

453056e

…ws-cdk into feature/stepfunctions/sagemaker

Merge branch 'main' into feature/stepfunctions/sagemaker

272ac0d

tmyoda requested a review from lpizzinidev August 12, 2023 22:38

lpizzinidev reviewed Aug 13, 2023

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/base-types.ts Outdated Show resolved Hide resolved

lpizzinidev suggested changes Aug 13, 2023

View reviewed changes

fix: renamed FASTFILE to FAST_FILE

9f2bb76

tmyoda requested a review from lpizzinidev August 14, 2023 18:28

lpizzinidev approved these changes Aug 15, 2023

View reviewed changes

Merge branch 'main' into feature/stepfunctions/sagemaker

5383117

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 15, 2023

kaizencc changed the title ~~fix(stepfunctions-tasks): FastFile mode for SageMaker Training Job~~ feat(stepfunctions-tasks): FastFile mode for SageMaker Training Job Aug 16, 2023

aws-cdk-automation previously requested changes Aug 16, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 16, 2023

kaizencc requested changes Aug 16, 2023

View reviewed changes

kaizencc and others added 3 commits August 16, 2023 16:00

Apply suggestions from code review

014009c

removed algorithm name validation

1a7fa59

Merge branch 'feature/stepfunctions/sagemaker' of github.com:tmyoda/a…

985def8

…ws-cdk into feature/stepfunctions/sagemaker

tmyoda requested a review from kaizencc August 19, 2023 19:35

kaizencc approved these changes Aug 23, 2023

View reviewed changes

Merge branch 'main' into feature/stepfunctions/sagemaker

baf02cb

kaizencc approved these changes Aug 23, 2023

View reviewed changes

mergify bot merged commit 5fef403 into aws:main Aug 23, 2023

tmyoda mentioned this pull request Aug 24, 2023

feat(stepfunctions-tasks): algorithmName validation for SageMakerCreateTrainingJob #26877

Merged

github-actions bot mentioned this pull request Sep 1, 2023

Monthly PRs metrics report - Aug 2023 #26965

Closed

Conversation

tmyoda commented Aug 9, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

tmyoda commented Aug 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

tmyoda Aug 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmyoda commented Aug 19, 2023

Uh oh!

mergify bot commented Aug 23, 2023

Uh oh!

aws-cdk-automation commented Aug 23, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants