Skip to content
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(aws-lambda-sagemakerendpoint): new pattern aws-lambda-sagemakerendpoint implementation #112

Merged
merged 63 commits into from
Feb 22, 2021

Conversation

tabdunabi
Copy link
Contributor

@tabdunabi tabdunabi commented Dec 22, 2020

  • Implementation of new pattern aws-lambda-sagemakerendpoint
  • Add SAGEMAKER_RUNTIME VPC Interface to vpc-helper.ts
  • Add SageMaker Endpoint default properties to sagemaker-defaults.ts
  • Add SageMaker Endpoint helper functions to sagemaker-helper.ts
  • Issues: Closes New Pattern: aws-lambda-sagemakerendpoint #111

@tabdunabi tabdunabi requested a review from hnishar as a code owner December 22, 2020 03:33
@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
  • Commit ID: 6491555
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
  • Commit ID: 5ed64da
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
  • Commit ID: 0097dd8
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
  • Commit ID: 0365515
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
  • Commit ID: 3508b91
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
  • Commit ID: 602ff1f
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
  • Commit ID: de4fd41
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-QfXHf1ULVFb1
  • Commit ID: 4880bb4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-QfXHf1ULVFb1
  • Commit ID: 08eed2b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@biffgaut biffgaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor issues, one more conceptual question/request.

- Enable reusing connections with Keep-Alive for NodeJs Lambda function.
- Allow the function to invoke the SageMaker endpoint for Inferences.
- Configure the function to access resources in the VPC, where the SageMaker endpoint is deployed.
- Enable X-Ray Tracing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the environment variable documented on the README page - am I missing it somewhere? Model after this:

AWS Lambda Function

  • Configure limited privilege access IAM role for Lambda function.
  • Enable reusing connections with Keep-Alive for NodeJs Lambda function.
  • Allow the function to only send messages to the queue (purging can be enabled using the enableQueuePurge property).
  • Enable X-Ray tracing
  • Set environment variables:
    • SQS_QUEUE_URL
    • AWS_NODEJS_CONNECTION_REUSE_ENABLED (for Node 10.x and higher functions)

| existingVpc? | `ec2.IVpc` | An optional, existing VPC into which this construct should be deployed. When deployed in a VPC, the Lambda function and Sagemaker Endpoint will use ENIs in the VPC to access network resources. An Interface Endpoint will be created in the VPC for Amazon Sagemaker Runtime, and Amazon S3 VPC Endpoint. If an existing VPC is provided, the `deployVpc?` property cannot be `true`. |
| vpcProps? | `ec2.VpcProps` | Optional user-provided properties to override the default properties for the new VPC. `enableDnsHostnames`, `enableDnsSupport`, `natGateways` and `subnetConfiguration` are set by the Construct, so any values for those properties supplied here will be overrriden. If `deployVpc?` is not `true` then this property will be ignored. |
| deployVpc? | `boolean` | Whether to create a new VPC based on defaults properties, or user-provided `vpcProps`, into which to deploy this construct. Setting this to `true` will deploy the minimal, most private VPC to run the construct. By default, the resources will be deployed in isolated subnets. If the `deployNatGateWay?` is set to `true`, resources will be deployed in private subnets with two NatGateWays deployed in the public subnets. One isolated, or private, subnet in each Availability Zone used by the CDK program. `enableDnsHostnames` and `enableDnsSupport` will both be set to true. If this property is `true` then `existingVpc?` cannot be specified. Defualts to `false`. | |
| role? | `iam.Role` | Optional IAM role, with all required permissions, to be assumed by Sagemaker Service to create resources. The pattern creates an internal role with minimum required permissions. However, the customer can provided a custom role. If a role is provided, the `executionRoleArn` in [`sagemaker.CfnModelProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-sagemaker.CfnModelProps.html) must be also specified. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change sentence to "By default, the pattern will create a role with the minimum required permissions, but the client can provide a custom role with additional capabilities."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I will update the README.md with lambda's env. variables.
  • Since, we are going to remove the Role? property, I will move "By default, the pattern will create a role with the minimum required permissions, but the client can provide a custom role with additional capabilities." to modelProps docs.

| existingVpc? | `ec2.IVpc` | An optional, existing VPC into which this construct should be deployed. When deployed in a VPC, the Lambda function and Sagemaker Endpoint will use ENIs in the VPC to access network resources. An Interface Endpoint will be created in the VPC for Amazon Sagemaker Runtime, and Amazon S3 VPC Endpoint. If an existing VPC is provided, the `deployVpc?` property cannot be `true`. |
| vpcProps? | `ec2.VpcProps` | Optional user-provided properties to override the default properties for the new VPC. `enableDnsHostnames`, `enableDnsSupport`, `natGateways` and `subnetConfiguration` are set by the Construct, so any values for those properties supplied here will be overrriden. If `deployVpc?` is not `true` then this property will be ignored. |
| deployVpc? | `boolean` | Whether to create a new VPC based on defaults properties, or user-provided `vpcProps`, into which to deploy this construct. Setting this to `true` will deploy the minimal, most private VPC to run the construct. By default, the resources will be deployed in isolated subnets. If the `deployNatGateWay?` is set to `true`, resources will be deployed in private subnets with two NatGateWays deployed in the public subnets. One isolated, or private, subnet in each Availability Zone used by the CDK program. `enableDnsHostnames` and `enableDnsSupport` will both be set to true. If this property is `true` then `existingVpc?` cannot be specified. Defualts to `false`. | |
| role? | `iam.Role` | Optional IAM role, with all required permissions, to be assumed by Sagemaker Service to create resources. The pattern creates an internal role with minimum required permissions. However, the customer can provided a custom role. If a role is provided, the `executionRoleArn` in [`sagemaker.CfnModelProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-sagemaker.CfnModelProps.html) must be also specified. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bigger question - we're asking the client to provide the same information twice. We specify the role here and the role arn in modelProps. Can't we drop this and get the role using fromResource on the modelProps arn? One of things we like to avoid is pulling out individual properties from CDK defined props and promoting them to the construct props. (but I like not having to create the role in advance :-) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the Role argument and use iam.Role.fromRoleArn to import customer's role using executionRoleArn provided in modelProps.

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-QfXHf1ULVFb1
  • Commit ID: b4bb92f
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@tabdunabi tabdunabi force-pushed the feat/aws-lambda-sagemakerendpoint branch from b4bb92f to 45387f6 Compare February 9, 2021 23:23
@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-QfXHf1ULVFb1
  • Commit ID: 45387f6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@@ -105,14 +105,16 @@ Out of the box implementation of the Construct without any override will set the
- Allow the function to invoke the Sagemaker endpoint for Inferences.
- Configure the function to access resources in the VPC, where the Sagemaker endpoint is deployed.
- Enable X-Ray Tracing.
- Set environment variables:
- `SAGEMAKER_ENDPOINT_NAME`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the `` and the period, make it look like all the others.

@@ -100,7 +93,7 @@ export class LambdaToSagemakerEndpoint extends cdk.Construct {
* @param {cdk.App} scope - represents the scope for all the resources.
* @param {string} id - this is a scope-unique id.
* @param {LambdaToSagemakerEndpointProps} props - user provided props for the construct.
* @since 1.76.0
* @since 1.86.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.87.0 is already out. This should be 1.87.1 (we'll hold off that release for this).

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-QfXHf1ULVFb1
  • Commit ID: 5e9fd53
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-QfXHf1ULVFb1
  • Commit ID: 123c265
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@biffgaut biffgaut dismissed hnishar’s stale review February 22, 2021 21:04

Items were all addressed

@biffgaut biffgaut merged commit ea4ab3b into awslabs:master Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Pattern: aws-lambda-sagemakerendpoint
5 participants