feat(stepfunctions-tasks): support for sagemaker tasks #6639

Hari-Nagarajan · 2020-03-09T17:49:07Z

Description

I needed support for these SageMaker endpoints for a project I'm working on. I verified this works by using this in my project to create a workflow that creates a training job, updates the model and then updates the endpoint.

Commit Message

feat(stepfunctions-tasks): support for sagemaker tasks (#6639)

closes #6572

End Commit Message

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

Closes: aws#6572 Added: - createModel - createEndpointConfig - createEndpoint - updateEndpoint

aws-cdk-automation · 2020-03-09T17:52:02Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0c3b26a
Result: FAILED
Build Logs (available for 30 days)

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

…an/aws-cdk into sfn-sagemaker-tasks

aws-cdk-automation · 2020-03-09T18:35:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: dcc20b8
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-09T18:47:13Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7424301
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-09T20:01:03Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 96de9c8
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-10T14:58:22Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e820ce2
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-11T06:55:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b0de38f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-11T14:45:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: d9352d0
Result: SUCCEEDED
Build Logs (available for 30 days)

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

Stacy-D · 2020-03-11T15:27:52Z

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

+ *
+ * @experimental
+ */
+export interface ProductionVariants {


This interface should be renamed to ProductionVariant instead of ProductionVariants, as it defines one ProductionVariant.
Also defining this interface here will introduce code duplication when PR for L2 sagemaker constructs is merged and ProductionVariant gets into the package. After mentioned PR being live, created step function tasks should use the real defined ProductionVariant in sagemaker instead of introducing another one.

Stacy-D · 2020-03-11T15:32:13Z

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

+ *
+ * @experimental
+ */
+export interface ModelContainer {


This interface should be named ContainerDefinition instead of ModelContainer to be consistent with sagemaker api
Also it should be replaced with ContainerDefinition in sagemaker constructs when #6107 is merged to avoid code duplication

Stacy-D · 2020-03-11T15:33:30Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+    readonly endpointConfigName: string;
+
+    /**
+     * Isolates the model container. No inbound or outbound network calls can be made to or from the model container.


This comment is not relevant to the below property

Stacy-D · 2020-03-11T15:34:50Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+    }
+
+    private makePolicyStatements(task: sfn.Task): iam.PolicyStatement[] {
+        Stack.of(task);


Stack.of(task); is unused in this methods context

Stacy-D · 2020-03-11T15:44:27Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+        const policyStatements = [
+            new iam.PolicyStatement({
+                actions: ['sagemaker:CreateEndpointConfig'],
+                resources: ['*'],
+            }),
+            new iam.PolicyStatement({
+                actions: ['sagemaker:ListTags'],
+                resources: ['*']
+            }),
+        ];


It is better to define the resources as follows

const stack = Stack.of(task); const policyStatements = [ new PolicyStatement({ actions: ['sagemaker:CreateEndpointConfig'], resources: [ stack.formatArn({ service: 'sagemaker', resource: 'endpoint-config', resourceName: '*', }), ], }), new PolicyStatement({ actions: ['sagemaker:ListTags'], resources: [ stack.formatArn({ service: 'sagemaker', resource: 'endpoint-config', resourceName: '*', }), ], }), ];

+1 but I'll also add - given that we know the name of the endpoint config at this point, we should be able to add that to resourceName and avoid *.

I was going to suggest specifying the resource name as well similar to:

new PolicyStatement({ actions: ['sagemaker:CreateEndpointConfig'], resources: [ stack.formatArn({ service: 'sagemaker', resource: 'endpoint-config', // resource name in sagemaker is lowercase resourceName: Data.isJsonPathString(this.props.endpointConfigName) ? '*' : `${this.props.endpointConfigName.toLowerCase()}`, }), ], }),

Please note that it is essential to convert resource name to lower case as Sagemaker arns lowercase the name by default(for any resource be it training job, endpoint, ,model etc). I found this case undocumented

Stacy-D · 2020-03-11T15:47:18Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-task.ts

+    }
+
+    private makePolicyStatements(task: sfn.Task): iam.PolicyStatement[] {
+        Stack.of(task);


This line is unused.

Stacy-D · 2020-03-11T15:48:15Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-task.ts

+        const policyStatements = [
+            new iam.PolicyStatement({
+                actions: ['sagemaker:createEndpoint'],
+                resources: ['*'],
+            }),
+            new iam.PolicyStatement({
+                actions: ['sagemaker:ListTags'],
+                resources: ['*']
+            })
+        ];


Refer to comment for EndpointConfig task

Stacy-D · 2020-03-11T15:49:51Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-model-task.ts

+    }
+
+    private makePolicyStatements(task: sfn.Task): iam.PolicyStatement[] {
+        Stack.of(task);


unused line

Stacy-D · 2020-03-11T15:52:09Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-update-endpoint-task.ts

+    }
+
+    private makePolicyStatements(task: sfn.Task): iam.PolicyStatement[] {
+        Stack.of(task);


unused line

Stacy-D · 2020-03-11T15:54:03Z

This PR will introduce code repetition if merged before #6107 as it defines interfaces that should come from L2 sagemaker constructs

P.S. I've left my suggestions on potential improvements as I've also implemented those tasks as they were required for my project

nija-at

Thanks for submitting this PR. This is going to add much value to CDK customers when it's made available.

Given that this is a big one, I wanted to make you aware that this is going to take several iterations and several weeks for it to be ready to be merged.

nija-at · 2020-03-12T09:59:34Z

packages/@aws-cdk/aws-stepfunctions/README.md

+* `tasks.SagemakerCreateModelTask` -- create a SageMaker model
+* `tasks.SagemakerCreateEndpointConfigTask` -- create a SageMaker Endpoint Config
+* `tasks.SagemakerCreateEndpointTask` -- create a new SageMaker Endpoint
+* `tasks.SagemakerUpdateEndpointTask` -- update an existing SageMaker Endpoint


Please provide examples for each of these. (see below)

nija-at · 2020-03-12T10:01:24Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+import { Stack } from '@aws-cdk/core';
+import { getResourceArn } from './resource-arn-suffix';
+import { DataCaptureConfig, ProductionVariants } from './sagemaker-task-base-types';
+


Switch to using 2 space indent everywhere. From what I can see the rest of the stepfunction-tasks package uses 2-space indent. Follow the same consistency.

nija-at · 2020-03-12T10:04:32Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+import { DataCaptureConfig, ProductionVariants } from './sagemaker-task-base-types';
+
+/**
+ *  @experimental


Add the following text and to all other modules to which this apply.

Suggested change

* @experimental

* @experimental - since the API may change when higher level constructs in the `aws-sagemaker` module is available

nija-at · 2020-03-12T10:07:49Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+    /**
+     * The request accepts the following data in JSON format.
+     *
+     * @default - None
+     */
+    readonly dataCaptureConfig?: DataCaptureConfig;


I can't see this as a supported parameter here - https://docs.aws.amazon.com/step-functions/latest/dg/connect-sagemaker.html

https://docs.aws.amazon.com/sagemaker/latest/APIReference/API_CreateEndpointConfig.html

I see it here.

Not all parameters of the API is supported by step functions. You can only configure the ones that are explicitly mentioned in the stepfunctions document (that I linked above).

It seems to me that DataCaptureConfig is not supported.

nija-at · 2020-03-12T10:11:30Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+    /**
+     * The service integration pattern indicates different ways to call SageMaker APIs.
+     *
+     * The valid value is FIRE_AND_FORGE.


nija-at · 2020-03-12T10:14:02Z

packages/@aws-cdk/aws-stepfunctions-tasks/package.json

@@ -129,6 +129,51 @@
  "stability": "experimental",
  "awslint": {
    "exclude": [
+      "props-default-doc:@aws-cdk/aws-stepfunctions-tasks.SagemakerUpdateEndpointTaskProps.retainAllVariantProperties",


Please address all of these exclusions. The general guidance is no new exclusions should be added. It should be rare that we add one here.

nija-at · 2020-03-12T10:17:08Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-create-endpoint-config-task.ts

+        const policyStatements = [
+            new iam.PolicyStatement({
+                actions: ['sagemaker:CreateEndpointConfig'],
+                resources: ['*'],
+            }),
+            new iam.PolicyStatement({
+                actions: ['sagemaker:ListTags'],
+                resources: ['*']
+            }),
+        ];


+1 but I'll also add - given that we know the name of the endpoint config at this point, we should be able to add that to resourceName and avoid *.

nija-at · 2020-03-12T10:23:04Z

Thanks for your comments on here @Stacy-D. We appreciate all contributions that keep our CDK repo healthy and updated. 😊

This PR will introduce code repetition if merged before #6107 as it defines interfaces that should come from L2 sagemaker constructs

I wouldn't be too worried about that yet. It's not clear when PR #6107 would land.
We're intentionally marking the constructs here as experimental, so we can go and bring in the Sagemaker constructs into this library, when it's ready. This way we can provide CDK customers the ability to still use sagemaker via stepfunctions, if they so wish to, with the warning that it is subject to change in future versions.
Hope that makes sense!

Hari-Nagarajan · 2020-03-13T17:09:28Z

Thanks for submitting this PR. This is going to add much value to CDK customers when it's made available.

Given that this is a big one, I wanted to make you aware that this is going to take several iterations and several weeks for it to be ready to be merged.

I'll put out an iteration soon.

github-actions · 2020-05-20T06:07:06Z

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

Hari Nagarajan and others added 4 commits March 5, 2020 16:24

feat(stepfunctions-tasks): add sagemaker endpoints

3664f1a

Closes: aws#6572 Added: - createModel - createEndpointConfig - createEndpoint - updateEndpoint

Fix linting and build

4dfb521

Add tests!

dcc20b8

Merge branch 'master' into sfn-sagemaker-tasks

0c3b26a

Hari Nagarajan added 2 commits March 9, 2020 13:53

Update README.md

cb1435f

Merge branch 'sfn-sagemaker-tasks' of https://github.com/Hari-Nagaraj…

7424301

…an/aws-cdk into sfn-sagemaker-tasks

SomayaB assigned nija-at Mar 9, 2020

Hari-Nagarajan marked this pull request as ready for review March 9, 2020 18:35

Merge branch 'master' into sfn-sagemaker-tasks

96de9c8

Merge branch 'master' into sfn-sagemaker-tasks

e820ce2

Merge branch 'master' into sfn-sagemaker-tasks

b0de38f

Merge branch 'master' into sfn-sagemaker-tasks

d9352d0

Stacy-D reviewed Mar 11, 2020

View reviewed changes

nija-at suggested changes Mar 12, 2020

View reviewed changes

nija-at changed the title ~~feat(stepfunctions-tasks): support for SageMaker createModel, createEndpointConfig, createEndpoint and updateEndpoint~~ feat(stepfunctions-tasks): support for sagemaker tasks Mar 12, 2020

aws deleted a comment from mergify bot May 19, 2020

nija-at added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 19, 2020

github-actions bot closed this May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stepfunctions-tasks): support for sagemaker tasks #6639

feat(stepfunctions-tasks): support for sagemaker tasks #6639

Hari-Nagarajan commented Mar 9, 2020 •

edited by nija-at

Loading

aws-cdk-automation commented Mar 9, 2020

aws-cdk-automation commented Mar 9, 2020

aws-cdk-automation commented Mar 9, 2020

aws-cdk-automation commented Mar 9, 2020

aws-cdk-automation commented Mar 10, 2020

aws-cdk-automation commented Mar 11, 2020

aws-cdk-automation commented Mar 11, 2020

Stacy-D Mar 11, 2020

Stacy-D Mar 11, 2020

Stacy-D Mar 11, 2020

Stacy-D Mar 11, 2020

Stacy-D Mar 11, 2020

nija-at Mar 12, 2020

Stacy-D Mar 12, 2020

Stacy-D Mar 11, 2020

Stacy-D Mar 11, 2020

Stacy-D Mar 11, 2020

Stacy-D Mar 11, 2020

Stacy-D commented Mar 11, 2020 •

edited

Loading

nija-at left a comment

nija-at Mar 12, 2020

nija-at Mar 12, 2020

nija-at Mar 12, 2020

nija-at Mar 12, 2020

Hari-Nagarajan Mar 13, 2020

nija-at Mar 13, 2020 •

edited

Loading

nija-at Mar 12, 2020

nija-at Mar 12, 2020

nija-at Mar 12, 2020

nija-at commented Mar 12, 2020

Hari-Nagarajan commented Mar 13, 2020 •

edited

Loading

github-actions bot commented May 20, 2020

	* @experimental
	* @experimental - since the API may change when higher level constructs in the `aws-sagemaker` module is available

feat(stepfunctions-tasks): support for sagemaker tasks #6639

feat(stepfunctions-tasks): support for sagemaker tasks #6639

Conversation

Hari-Nagarajan commented Mar 9, 2020 • edited by nija-at Loading

Description

Commit Message

End Commit Message

aws-cdk-automation commented Mar 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 11, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stacy-D commented Mar 11, 2020 • edited Loading

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at commented Mar 12, 2020

Hari-Nagarajan commented Mar 13, 2020 • edited Loading

github-actions bot commented May 20, 2020

Hari-Nagarajan commented Mar 9, 2020 •

edited by nija-at

Loading

Stacy-D commented Mar 11, 2020 •

edited

Loading

nija-at Mar 13, 2020 •

edited

Loading

Hari-Nagarajan commented Mar 13, 2020 •

edited

Loading