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(sagemaker): add EndpointConfig L2 construct #22865

Merged
merged 8 commits into from
Nov 11, 2022

Conversation

petermeansrock
Copy link
Contributor

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

related to #2809


All Submissions:

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 [email protected]
Co-authored-by: Long Yao [email protected]
Co-authored-by: Drew Jetter [email protected]
Co-authored-by: Murali Ganesh [email protected]
Co-authored-by: Abilash Rangoju [email protected]

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 <[email protected]>
Co-authored-by: Long Yao <[email protected]>
Co-authored-by: Drew Jetter <[email protected]>
Co-authored-by: Murali Ganesh <[email protected]>
Co-authored-by: Abilash Rangoju <[email protected]>
@gitpod-io
Copy link

gitpod-io bot commented Nov 10, 2022

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Nov 10, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team November 10, 2022 17:11
@petermeansrock petermeansrock marked this pull request as ready for review November 10, 2022 17:59
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

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

packages/@aws-cdk/aws-sagemaker/lib/endpoint-config.ts Outdated Show resolved Hide resolved
if (props.variantName in this._instanceProductionVariants) {
throw new Error(`There is already a Production Variant with name '${props.variantName}'`);
}
this.validateProps(props);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@petermeansrock petermeansrock Nov 10, 2022

Choose a reason for hiding this comment

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

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.

Comment on lines 247 to 249
public get instanceProductionVariants(): InstanceProductionVariant[] {
return Object.values(this._instanceProductionVariants);
}
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 understand the need for a getter and to differentiate between instanceProductionVariants and _instanceProductionVariants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@petermeansrock petermeansrock Nov 10, 2022

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-sagemaker/lib/private/util.ts Outdated Show resolved Hide resolved
@petermeansrock petermeansrock marked this pull request as draft November 10, 2022 22:54
@mergify mergify bot dismissed kaizencc’s stale review November 10, 2022 23:08

Pull request has been modified.

@petermeansrock petermeansrock marked this pull request as ready for review November 11, 2022 00:06
Comment on lines +249 to +253
* @internal
*/
public get _instanceProductionVariants(): InstanceProductionVariant[] {
return Object.values(this.instanceProductionVariantsByName);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@petermeansrock petermeansrock Nov 11, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Hi @petermeansrock! This looks good to me!

@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2022

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
Copy link
Collaborator

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 mergify bot merged commit 0e97c15 into aws:main Nov 11, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2022

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

@petermeansrock petermeansrock deleted the sagemaker-l2-endpointconfig branch November 11, 2022 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants