feat(sagemaker): Initial version of the L2 construct for SageMaker #2888

mattmcclean · 2019-06-17T13:55:45Z

I have written up the initial L2 Construct for SageMaker. It is quite basic and there can be further enhancements made over time such as adding support for Autoscaling of the endpoint.

It creates 3 L2 resources including:

NotebookInstance - creates the SageMaker NotebookInstance and associated LifecycleConfig CFN resources
Model - creates the SageMaker Model CFN resource
Endpoint - creates the Endpoint + EndpointConfig CFN resources

Fixes #2809

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Merged upstream

skinny85 · 2019-08-05T23:36:28Z

Sorry for leaving this PR out in the cold @mattmcclean . Unfortunately, you submitted it right before our mad dash to release 1.0.0.

Now that that's over, I'll be taking over the PR. Expect a review soon.

Thanks, and apologies again.

skinny85

This is an extremely high-quality contribution @mattmcclean , thank you so much!

I have some comments, but most of them are from conventions in the Construct Library that we follow, but maybe haven't documented (and are not checking with our linter) well enough.

One big change though: we use 2 spaces for indentation in our code. I understand 4 is the default in most tools, but could you please change it to 2, and re-format the code?

Thanks again for the awesome contribution!

skinny85 · 2019-08-06T18:07:02Z

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

@@ -15,6 +15,78 @@
 ---
 <!--END STABILITY BANNER-->



We usually like to start the ReadMe with a short blurb about the service itself (usually copied from AWS documentation), and then installation and import instructions. See CodeBuild for an example.

So, this should be something like:

Amazon SageMaker provides every developer and data scientist with the ability to build, train, and deploy machine learning models quickly. Amazon SageMaker is a fully-managed service that covers the entire machine learning workflow to label and prepare your data, choose an algorithm, train the model, tune and optimize it for deployment, make predictions, and take action. Your models get to production faster with much less effort and lower cost.

Installation

Install the module:

$ npm i @aws-cdk/aws-sagemaker

Import it into your code:

import sagemaker = require('@aws-cdk/aws-sagemaker');

skinny85 · 2019-08-06T18:08:10Z

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

+new NotebookInstance(this, 'MyNotebook', {
+    vpc,
+    kmsKeyId: key,
+    instanceType = new ec2.InstanceType('p3.2xlarge'),


Can you use the InstanceType.of method here instead? It's a little "nicer" I think :)

skinny85 · 2019-08-06T18:08:47Z

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

+new NotebookInstance(this, 'MyNotebook');
+```
+
+Add a KMS encryption key, launch in own VPC, set the instance type to 'ml.p3.xlarge', disable direct internet access and set the EBS volume size to 100 GB.


This sentence should probably should end with a colon instead of a period.

skinny85 · 2019-08-06T18:09:00Z

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

+});
+```
+
+Add custom scripts when notebook instance is created and started.


Same here (period -> colon).

skinny85 · 2019-08-06T18:09:14Z

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

+
+### Models
+
+Define a model.


skinny85 · 2019-08-06T20:28:21Z

packages/@aws-cdk/aws-sagemaker/lib/endpoint.ts

+        const result = super.validate();
+        // check we have at least one production variant
+        if (this.productionVariants.length === 0) {
+            result.push("Must have at least one Production Variant");


So here's an interesting pattern that you can use here.

Since at least one ProductionVariant is required, have in props:

productionVariant: ProductionVariant; // required extraProductionVariants?: ProductionVariant[]; // optional

This way, you communicate to the clients of this class, at compile time, that at least one variant has to be provided.

skinny85 · 2019-08-06T20:29:13Z

packages/@aws-cdk/aws-sagemaker/lib/endpoint.ts

+     * Validates the provided instance type.
+     * @param instanceType the instance type of the SageMaker endpoint production variant.
+     */
+    private validateInstanceType(instanceType: string, result: string[]) {


Just have this return string[], and merge the errors in the client of this method, instead of mutating a parameter. I think keeping methods pure is simply easier.

skinny85 · 2019-08-06T20:30:20Z

packages/@aws-cdk/aws-sagemaker/lib/endpoint.ts

+     */
+    private validateInstanceType(instanceType: string, result: string[]) {
+        // check if a valid sagemaker instance type
+        if (!['c4', 'c5', 'm4', 'm5', 'p2', 'p3', 't2'].some(instanceClass => instanceType.indexOf(instanceClass) >= 0)) {


This is exactly the same as in Notebook - extract to a module-private function (and include the allowed types in the error message).

skinny85 · 2019-08-06T20:31:56Z

packages/@aws-cdk/aws-sagemaker/test/test.endpoint.ts

+            });
+
+            // THEN
+            expect(stack).to(haveResource("AWS::SageMaker::EndpointConfig", {


General comment for all tests: we have also a haveResourceLike matcher that allows you to check only the parts of the resource that you're interested in in a given test. Might come in handy for these.

skinny85 · 2019-08-06T20:39:55Z

packages/@aws-cdk/aws-sagemaker/test/test.endpoint.ts

+
+            test.done();
+        },
+        "it throws error when incorrect variant weight given"(test: Test) {


Please always have an empty line between tests on the same level of indentation.

skinny85 · 2019-08-06T20:48:13Z

packages/@aws-cdk/aws-sagemaker/test/test.endpoint.ts

+            test.equal(errors.length, 1);
+            const error = errors[0];
+            test.same(error.source, endpoint);
+            test.equal(error.message, "Must have at least one Production Variant");


You can simplify this with something like:

test.deepEqual(errors.map(e => e.message), ["Must have at least one Production Variant"]);

Same comment for all tests dealing with error checking :)

mergify · 2019-11-01T19:42:35Z