feat(sagemaker): add EndpointConfig L2 construct #22865

petermeansrock · 2022-11-10T17:11:18Z

This is the second of three PRs to complete the implementation of RFC 431:
aws/aws-cdk-rfcs#431

related to #2809

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

Co-authored-by: Matt McClean mmcclean@amazon.com
Co-authored-by: Long Yao yl1984108@gmail.com
Co-authored-by: Drew Jetter 60628154+jetterdj@users.noreply.github.com
Co-authored-by: Murali Ganesh 59461079+foxpro24@users.noreply.github.com
Co-authored-by: Abilash Rangoju 988529+rangoju@users.noreply.github.com

This is the second of three PRs to complete the implementation of RFC 431: aws/aws-cdk-rfcs#431 related to aws#2809 Co-authored-by: Matt McClean <mmcclean@amazon.com> Co-authored-by: Long Yao <yl1984108@gmail.com> Co-authored-by: Drew Jetter <60628154+jetterdj@users.noreply.github.com> Co-authored-by: Murali Ganesh <59461079+foxpro24@users.noreply.github.com> Co-authored-by: Abilash Rangoju <988529+rangoju@users.noreply.github.com>

gitpod-io · 2022-11-10T17:11:25Z

kaizencc

Hi @petermeansrock! In general looking pretty good! Testing coverage is especially nice :).

packages/@aws-cdk/aws-sagemaker/lib/accelerator-type.ts

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

kaizencc · 2022-11-10T21:07:09Z

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

+    if (props.variantName in this._instanceProductionVariants) {
+      throw new Error(`There is already a Production Variant with name '${props.variantName}'`);
+    }
+    this.validateProps(props);


Suggested change

this.validateProps(props);

this.validateInstanceProductionVariantProps(props);

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

kaizencc · 2022-11-10T21:12:39Z

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

+   * Find instance production variant based on variant name
+   * @param name Variant name from production variant
+   */
+  public findInstanceProductionVariant(name: string): InstanceProductionVariant {


When/how will this public API be used? Might benefit from a readme example.

~~This method will be used by the upcoming Endpoint L2 construct to examine the variants configured on its associated EndpointConfig resource.~~

Actually, now that I say that, there's no reason these need to be exposed outside of the SageMaker module. I'm going to mark these Endpoint-only methods as @internal to avoid exposing them.

kaizencc · 2022-11-10T21:47:51Z

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

+  public get instanceProductionVariants(): InstanceProductionVariant[] {
+    return Object.values(this._instanceProductionVariants);
+  }


I don't understand the need for a getter and to differentiate between instanceProductionVariants and _instanceProductionVariants.

The member _instanceProductionVariants is a map from variant name to variant (of type { [key: string]: InstanceProductionVariant; }) to aid with lookup operations by variant name (and avoid variant name duplication) whereas this getter is exposing the variants as an array (for use-cases in which consuming code may wish to examine all configured variants).

After thinking about this a bit more in light of my marking Endpoint-only methods as @internal, I've decided to make the following changes:

I've renamed _instanceProductionVariants to instanceProductionVariantsByName to make its purpose as a map clearer.

The now-@internal methods are prefixed with _ to obey linting rules.

The @internal getter is still useful so that Endpoint can itself expose a list of configured variants, but it need not be exposed to customers..

packages/@aws-cdk/aws-sagemaker/lib/private/util.ts

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

Pull request has been modified.

kaizencc · 2022-11-11T16:30:21Z

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

+   * @internal
+   */
+  public get _instanceProductionVariants(): InstanceProductionVariant[] {
+    return Object.values(this.instanceProductionVariantsByName);
+  }


lets have a conversation about @internal public APIs here, and have it apply to the rest of the PR. The two that i'm noticing, although there may be more, is _instanceProdVariants() and _findInstanceProdVariants()

_instanceProdVariants() doesn't seem to be used outside of the construct, so why is it public in the first place?
_findInstanceProdVariants() is being used outside the construct in testing only.

I don't really like using this pattern for the express purpose of utilizing the API in testing, as it seems to be the case here. Generally, I'm okay with @internal only if it is necessary for a different construct to consume the method. From what I can see, the tests may have to be clunkier, but we can assert on the resulting template itself rather than using this getter and avoid synthing. I'm likely missing something in your reasoning for using @internal, so let me know your thought process and see if we can come to a reasonable solution.

Pre-empting your (possible) response: is this being used in Endpoint? If so, can you explain a bit about how/why?

Yes, these are being used by Endpoint:

These @internal methods are used by similarly named functions on Endpoint named get instanceProductionVariants and findInstanceProductionVariant (see this section of the RFC).

This other section of the RFC describes the purpose behind these methods.

The AutoScaling and Metrics sections of the RFC's README includes examples using this feature.

In short, as variants deployed to an endpoint represent an auto-scalable and monitorable entity, Endpoint inspects the supplied EndpointConfig's variants to offer metric* and autoScale* APIs with synthesis-time safeguards to ensure that customers are in fact monitoring existing variants.

If you'd like, I can rip these @internal methods out of this PR and include them in the Endpoint PR where their purpose is clearer (as of now, in this PR, they are simply being tested and serve no other purpose).

Nope. This is great, and sorry I know I could have found these resources by myself, but I was in a rush.

@internal is meant for when we need to reference a construct's methods in other constructs, and we do not wish to expose the API to the consumer. This is that case, so we are good with this method.

kaizencc

Hi @petermeansrock! This looks good to me!

mergify · 2022-11-11T19:38:05Z

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 · 2022-11-11T20:16:39Z

AWS CodeBuild CI Report

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

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

mergify · 2022-11-11T20:16:54Z

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

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Nov 10, 2022

aws-cdk-automation requested a review from a team November 10, 2022 17:11

Use allow-listed account ID for cross-env tests

49d49cb

petermeansrock marked this pull request as ready for review November 10, 2022 17:59

kaizencc previously requested changes Nov 10, 2022

View reviewed changes

petermeansrock marked this pull request as draft November 10, 2022 22:54

petermeansrock added 4 commits November 10, 2022 14:57

Specify @default instance using defined type

d7c8f3f

Fix util.ts doc typo

7f7a46f

Give validateProps a more descriptive name

47159b1

Drop all mentions of environment "dimension"

99b2bf0

Mark methods used only by Endpoint as @internal

a06f443

petermeansrock marked this pull request as ready for review November 11, 2022 00:06

kaizencc reviewed Nov 11, 2022

View reviewed changes

kaizencc approved these changes Nov 11, 2022

View reviewed changes

Merge branch 'main' into sagemaker-l2-endpointconfig

c226c87

mergify bot merged commit 0e97c15 into aws:main Nov 11, 2022

petermeansrock deleted the sagemaker-l2-endpointconfig branch November 11, 2022 20:17

kaizencc mentioned this pull request Nov 28, 2022

sagemaker: Create L2 construct for Amazon SageMaker #2809

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sagemaker): add EndpointConfig L2 construct #22865

feat(sagemaker): add EndpointConfig L2 construct #22865

petermeansrock commented Nov 10, 2022

gitpod-io bot commented Nov 10, 2022

kaizencc left a comment

kaizencc Nov 10, 2022

kaizencc Nov 10, 2022

petermeansrock Nov 10, 2022 •

edited

kaizencc Nov 10, 2022

petermeansrock Nov 10, 2022

petermeansrock Nov 10, 2022 •

edited

kaizencc Nov 11, 2022

kaizencc Nov 11, 2022

petermeansrock Nov 11, 2022 •

edited

petermeansrock Nov 11, 2022

kaizencc Nov 11, 2022

kaizencc left a comment

mergify bot commented Nov 11, 2022

aws-cdk-automation commented Nov 11, 2022

mergify bot commented Nov 11, 2022

	this.validateProps(props);
	this.validateInstanceProductionVariantProps(props);

feat(sagemaker): add EndpointConfig L2 construct #22865

feat(sagemaker): add EndpointConfig L2 construct #22865

Conversation

petermeansrock commented Nov 10, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Nov 10, 2022

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermeansrock Nov 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermeansrock Nov 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermeansrock Nov 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 11, 2022

aws-cdk-automation commented Nov 11, 2022

AWS CodeBuild CI Report

mergify bot commented Nov 11, 2022

petermeansrock Nov 10, 2022 •

edited

petermeansrock Nov 10, 2022 •

edited

petermeansrock Nov 11, 2022 •

edited