fix(stepfunctions-tasks): instance type for SageMakerCreateTrainingJob cannot be specified dynamically through JSONPath #15215

JonBlauvelt · 2021-06-20T02:44:08Z

SageMakerCreateTrainingJob accepts only a static InstanceType. Accept an InstanceType that contains a JsonPath so that the instance can be set dynamically at runtime.

Closes #11928

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-06-20T02:44:10Z

shivlaks

thanks for the contribution @JonBlauvelt - it would also be useful for discovery if we update the README to add a comment to the SageMakerCreateTrainingJob to include a comment with the JSONPath example.

shivlaks · 2021-06-20T17:08:09Z

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

    resourceConfig: {
      instanceCount: 1,
-      instanceType: ec2.InstanceType.of(ec2.InstanceClass.P3, ec2.InstanceSize.XLARGE2),
+      instanceType: new ec2.InstanceType(sfn.JsonPath.stringAt('$.TrainingJob.InstanceType')),
      volumeSize: cdk.Size.gibibytes(50),
      volumeEncryptionKey: kmsKey,


please add a new test for exercising the added control flow rather than to modify an existing test.
we want this one to stay as it is and ensure that it passes doesn't regress.

I'm happy to add a separate test case if required, but I'm not sure it ads any coverage. Here are the reasons why I went this route instead:

Stays consistent with CreateSageMakerEndpointConfig (added in this similar commit)

Test case for the path where InstanceType is provided at build time is already covered here.

It does add coverage because there are 2 scenarios that we want to test here wrt the generated Amazon States Language (ASL) definition:

providing a literal string as an instance type - the key will be InstanceType in the resource config

providing a dynamic reference as an instance type - the key will be InstanceType.$ in the resource config

The prior art here is less reusable because the classes that were initially introduced had an @experimental annotation, but since those were dropped recently, we want to take some additional measures to ensure we're not regressing.

by switching the test over from literal to dynamic, we're still only capturing 1 out of the 2 scenarios so ideally we have tests that cover both. Let me know if you have any questions.

discussed offline - both of these scenarios are covered in tests as other existing tests already cover the literal string use case.

JonBlauvelt · 2021-06-20T19:01:57Z

thanks for the contribution @JonBlauvelt - it would also be useful for discovery if we update the README to add a comment to the SageMakerCreateTrainingJob to include a comment with the JSONPath example.

Makes sense - I'll add that.

Pull request has been modified.

shivlaks

@JonBlauvelt thanks for contributing the 🐛 fix!! 🎉

Pull request has been modified.

shivlaks · 2021-06-22T16:56:56Z

@Mergifyio refresh

mergify · 2021-06-22T17:10:10Z

Command refresh: success

Pull request refreshed

mergify · 2021-06-22T17:10:13Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

BenChaimberg

Looks good! To further aid with discoverability, please modify the ResourceConfig docstring to add directions on how to pull it from the task input

aws-cdk/packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker/base-types.ts

Lines 206 to 211 in e5c0d59

    
             /** 
        
              * ML compute instance type. 
        
              * 
        
              * @default is the 'm4.xlarge' instance type. 
        
              */ 
        
             readonly instanceType: ec2.InstanceType;

BenChaimberg · 2021-06-22T17:15:52Z

oh and please add a summary of your changes in the PR description so it can be recorded in the merged commit

JonBlauvelt · 2021-06-22T17:58:26Z

oh and please add a summary of your changes in the PR description so it can be recorded in the merged commit

Done.

…eMakerCreateTrainingJob task, fixes #11928

JonBlauvelt · 2021-06-22T18:26:53Z

Looks good! To further aid with discoverability, please modify the ResourceConfig docstring to add directions on how to pull it from the task input

aws-cdk/packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker/base-types.ts

Lines 206 to 211 in e5c0d59

/**

* ML compute instance type.

*

* @default is the 'm4.xlarge' instance type.

*/

readonly instanceType: ec2.InstanceType;

Updated.

Pull request has been modified.

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker/base-types.ts

Pull request has been modified.

mergify · 2021-06-22T19:30:48Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-06-22T20:07:54Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 896b929
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-06-22T20:09:43Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…b cannot be specified dynamically through JSONPath (aws#15215) SageMakerCreateTrainingJob accepts only a static `InstanceType`. Accept an `InstanceType` that contains a `JsonPath` so that the instance can be set dynamically at runtime. Closes aws#11928 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

shivlaks previously requested changes Jun 20, 2021

View reviewed changes

shivlaks changed the title ~~fix(stepfunctions-tasks): Accept ec2.InstanceType of JsonPath for Sag…~~ fix(stepfunctions-tasks): instance type for SageMakerCreateTrainingJob cannot be specified dynamically through JSONPath Jun 20, 2021

shivlaks previously approved these changes Jun 22, 2021

View reviewed changes

BenChaimberg self-requested a review June 22, 2021 17:02

BenChaimberg previously requested changes Jun 22, 2021

View reviewed changes

Jon Blauvelt and others added 2 commits June 22, 2021 11:25

fix(stepfunctions-tasks): Accept ec2.InstanceType of JsonPath for Sag…

866e10b

…eMakerCreateTrainingJob task, fixes #11928

Merge branch 'master' into master

0c0e3aa

JonBlauvelt requested a review from BenChaimberg June 22, 2021 18:28

BenChaimberg suggested changes Jun 22, 2021

View reviewed changes

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

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

Documentation clarifications

12da7dd

BenChaimberg previously approved these changes Jun 22, 2021

View reviewed changes

BenChaimberg approved these changes Jun 22, 2021

View reviewed changes

Merge branch 'master' into master

896b929

mergify bot merged commit 9280d95 into aws:master Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions-tasks): instance type for SageMakerCreateTrainingJob cannot be specified dynamically through JSONPath #15215

fix(stepfunctions-tasks): instance type for SageMakerCreateTrainingJob cannot be specified dynamically through JSONPath #15215

JonBlauvelt commented Jun 20, 2021 •

edited

gitpod-io bot commented Jun 20, 2021

shivlaks left a comment

shivlaks Jun 20, 2021

JonBlauvelt Jun 20, 2021

shivlaks Jun 21, 2021

shivlaks Jun 22, 2021

JonBlauvelt commented Jun 20, 2021

shivlaks left a comment

shivlaks commented Jun 22, 2021

mergify bot commented Jun 22, 2021

mergify bot commented Jun 22, 2021

BenChaimberg left a comment

BenChaimberg commented Jun 22, 2021

JonBlauvelt commented Jun 22, 2021

JonBlauvelt commented Jun 22, 2021

mergify bot commented Jun 22, 2021

aws-cdk-automation commented Jun 22, 2021

mergify bot commented Jun 22, 2021

	/**
	* ML compute instance type.
	*
	* @default is the 'm4.xlarge' instance type.
	*/
	readonly instanceType: ec2.InstanceType;

fix(stepfunctions-tasks): instance type for SageMakerCreateTrainingJob cannot be specified dynamically through JSONPath #15215

fix(stepfunctions-tasks): instance type for SageMakerCreateTrainingJob cannot be specified dynamically through JSONPath #15215

Conversation

JonBlauvelt commented Jun 20, 2021 • edited

gitpod-io bot commented Jun 20, 2021

shivlaks left a comment

Choose a reason for hiding this comment

shivlaks Jun 20, 2021

Choose a reason for hiding this comment

JonBlauvelt Jun 20, 2021

Choose a reason for hiding this comment

shivlaks Jun 21, 2021

Choose a reason for hiding this comment

shivlaks Jun 22, 2021

Choose a reason for hiding this comment

JonBlauvelt commented Jun 20, 2021

shivlaks left a comment

Choose a reason for hiding this comment

shivlaks commented Jun 22, 2021

mergify bot commented Jun 22, 2021

mergify bot commented Jun 22, 2021

BenChaimberg left a comment

Choose a reason for hiding this comment

BenChaimberg commented Jun 22, 2021

JonBlauvelt commented Jun 22, 2021

JonBlauvelt commented Jun 22, 2021

mergify bot commented Jun 22, 2021

aws-cdk-automation commented Jun 22, 2021

AWS CodeBuild CI Report

mergify bot commented Jun 22, 2021

JonBlauvelt commented Jun 20, 2021 •

edited