Skip to content

Commit 6213979

Browse files
Mia Changpymia
authored andcommitted
refactor(sagemaker): address latest PR review feedback
- Remove redundant hasInstanceVariants variable in validation logic - Add SageMaker Serverless Inference documentation link to README - Throw error in renderServerlessProductionVariant when undefined - Add validation in renderInstanceProductionVariants for empty variants - Add comprehensive API assertions to integration tests - Update integration test snapshots Addresses review comments from abidhasan-aws in PR #35557
1 parent 395b5d5 commit 6213979

File tree

7 files changed

+147
-67
lines changed

7 files changed

+147
-67
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ const endpointConfig = new sagemaker.EndpointConfig(this, 'EndpointConfig', {
216216

217217
### Serverless Inference
218218

219-
Amazon SageMaker Serverless Inference is a purpose-built inference option that makes it easy for you to deploy and scale ML models. Serverless endpoints automatically launch compute resources and scale them in and out depending on traffic, eliminating the need to choose instance types or manage scaling policies.
219+
Amazon SageMaker Serverless Inference is a purpose-built inference option that makes it easy for you to deploy and scale ML models. Serverless endpoints automatically launch compute resources and scale them in and out depending on traffic, eliminating the need to choose instance types or manage scaling policies. For more information, see [SageMaker Serverless Inference](https://docs.aws.amazon.com/sagemaker/latest/dg/serverless-endpoints.html).
220220

221221
To create a serverless endpoint configuration, use the `serverlessProductionVariant` property:
222222

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -370,21 +370,20 @@ export class EndpointConfig extends cdk.Resource implements IEndpointConfig {
370370
}
371371

372372
private validateProductionVariants(): void {
373-
const hasInstanceVariants = this._instanceProductionVariants.length > 0;
374373
const hasServerlessVariant = this.serverlessProductionVariant !== undefined;
375374

376375
// validate at least one production variant
377-
if (!hasInstanceVariants && !hasServerlessVariant) {
376+
if (this._instanceProductionVariants.length === 0 && !hasServerlessVariant) {
378377
throw new Error('Must configure at least 1 production variant');
379378
}
380379

381380
// validate mutual exclusivity
382-
if (hasInstanceVariants && hasServerlessVariant) {
381+
if (this._instanceProductionVariants.length > 0 && hasServerlessVariant) {
383382
throw new Error('Cannot configure both instance and serverless production variants');
384383
}
385384

386385
// validate instance variant limits
387-
if (hasInstanceVariants && this._instanceProductionVariants.length > 10) {
386+
if (this._instanceProductionVariants.length > 10) {
388387
throw new Error('Can\'t have more than 10 production variants');
389388
}
390389
}
@@ -474,6 +473,10 @@ export class EndpointConfig extends cdk.Resource implements IEndpointConfig {
474473
* Render the list of instance production variants.
475474
*/
476475
private renderInstanceProductionVariants(): CfnEndpointConfig.ProductionVariantProperty[] {
476+
if (this._instanceProductionVariants.length === 0) {
477+
throw new Error('renderInstanceProductionVariants called but no instance variants are configured');
478+
}
479+
477480
return this._instanceProductionVariants.map( v => ({
478481
acceleratorType: v.acceleratorType?.toString(),
479482
initialInstanceCount: v.initialInstanceCount,
@@ -489,7 +492,7 @@ export class EndpointConfig extends cdk.Resource implements IEndpointConfig {
489492
*/
490493
private renderServerlessProductionVariant(): CfnEndpointConfig.ProductionVariantProperty[] {
491494
if (!this.serverlessProductionVariant) {
492-
return [];
495+
throw new Error('renderServerlessProductionVariant called but no serverless variant is configured');
493496
}
494497

495498
const variant = this.serverlessProductionVariant;

packages/@aws-cdk/aws-sagemaker-alpha/test/integ.endpoint-config.js.snapshot/aws-cdk-sagemaker-endpointconfig.assets.json

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk/aws-sagemaker-alpha/test/integ.endpoint-config.js.snapshot/aws-cdk-sagemaker-endpointconfig.template.json

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -739,6 +739,49 @@
739739
}
740740
]
741741
}
742+
},
743+
"MinimalServerlessEndpointConfig68CC9C3F": {
744+
"Type": "AWS::SageMaker::EndpointConfig",
745+
"Properties": {
746+
"ProductionVariants": [
747+
{
748+
"InitialVariantWeight": 1,
749+
"ModelName": {
750+
"Fn::GetAtt": [
751+
"ModelWithoutArtifactAndVpcModel9A8AD144",
752+
"ModelName"
753+
]
754+
},
755+
"ServerlessConfig": {
756+
"MaxConcurrency": 1,
757+
"MemorySizeInMB": 1024
758+
},
759+
"VariantName": "minimalServerlessVariant"
760+
}
761+
]
762+
}
763+
},
764+
"BoundaryServerlessEndpointConfig0B181608": {
765+
"Type": "AWS::SageMaker::EndpointConfig",
766+
"Properties": {
767+
"ProductionVariants": [
768+
{
769+
"InitialVariantWeight": 1,
770+
"ModelName": {
771+
"Fn::GetAtt": [
772+
"ModelWithoutArtifactAndVpcModel9A8AD144",
773+
"ModelName"
774+
]
775+
},
776+
"ServerlessConfig": {
777+
"MaxConcurrency": 200,
778+
"MemorySizeInMB": 6144,
779+
"ProvisionedConcurrency": 200
780+
},
781+
"VariantName": "boundaryServerlessVariant"
782+
}
783+
]
784+
}
742785
}
743786
},
744787
"Parameters": {

packages/@aws-cdk/aws-sagemaker-alpha/test/integ.endpoint-config.js.snapshot/manifest.json

Lines changed: 33 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk/aws-sagemaker-alpha/test/integ.endpoint-config.js.snapshot/tree.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 58 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,61 +1,11 @@
11
import * as path from 'path';
22
import * as ec2 from 'aws-cdk-lib/aws-ec2';
33
import * as cdk from 'aws-cdk-lib';
4-
import { IntegTest } from '@aws-cdk/integ-tests-alpha';
4+
import { IntegTest, ExpectedResult } from '@aws-cdk/integ-tests-alpha';
55
import * as sagemaker from '../lib';
66

77
/*
8-
* Stack verification steps:
9-
* aws sagemaker describe-endpoint-config --endpoint-config-name <endpoint config name>
10-
*
11-
* For instance-based endpoint config, the above command will result in the following output:
12-
* {
13-
* "EndpointConfigName": "EndpointConfig...",
14-
* "EndpointConfigArn": "arn:aws:sagemaker:...",
15-
* "ProductionVariants": [
16-
* {
17-
* "VariantName": "firstVariant",
18-
* "ModelName": "ModelWithArtifactAndVpcModel...",
19-
* "InitialInstanceCount": 1,
20-
* "InstanceType": "ml.m5.large",
21-
* "InitialVariantWeight": 1.0
22-
* },
23-
* {
24-
* "VariantName": "secondVariant",
25-
* "ModelName": "ModelWithArtifactAndVpcModel...",
26-
* "InitialInstanceCount": 1,
27-
* "InstanceType": "ml.t2.medium",
28-
* "InitialVariantWeight": 1.0
29-
* },
30-
* {
31-
* "VariantName": "thirdVariant",
32-
* "ModelName": "ModelWithoutArtifactAndVpcModel...",
33-
* "InitialInstanceCount": 1,
34-
* "InstanceType": "ml.t2.medium",
35-
* "InitialVariantWeight": 2.0
36-
* }
37-
* ],
38-
* "CreationTime": "..."
39-
* }
40-
*
41-
* For serverless endpoint config, the command will show:
42-
* {
43-
* "EndpointConfigName": "ServerlessEndpointConfig...",
44-
* "EndpointConfigArn": "arn:aws:sagemaker:...",
45-
* "ProductionVariants": [
46-
* {
47-
* "VariantName": "serverlessVariant",
48-
* "ModelName": "ModelWithoutArtifactAndVpcModel...",
49-
* "InitialVariantWeight": 1.0,
50-
* "ServerlessConfig": {
51-
* "MaxConcurrency": 10,
52-
* "MemorySizeInMB": 2048,
53-
* "ProvisionedConcurrency": 5
54-
* }
55-
* }
56-
* ],
57-
* "CreationTime": "..."
58-
* }
8+
* Stack verification is performed using API assertions below.
599
*/
6010

6111
const app = new cdk.App();
@@ -92,7 +42,7 @@ endpointConfig.addInstanceProductionVariant({
9242
});
9343

9444
// Test serverless endpoint configuration with all properties
95-
new sagemaker.EndpointConfig(stack, 'ServerlessEndpointConfig', {
45+
const serverlessEndpointConfig = new sagemaker.EndpointConfig(stack, 'ServerlessEndpointConfig', {
9646
serverlessProductionVariant: {
9747
model: modelWithoutArtifactAndVpc,
9848
variantName: 'serverlessVariant',
@@ -104,7 +54,7 @@ new sagemaker.EndpointConfig(stack, 'ServerlessEndpointConfig', {
10454
});
10555

10656
// Test serverless endpoint configuration with minimal properties
107-
new sagemaker.EndpointConfig(stack, 'MinimalServerlessEndpointConfig', {
57+
const minimalServerlessEndpointConfig = new sagemaker.EndpointConfig(stack, 'MinimalServerlessEndpointConfig', {
10858
serverlessProductionVariant: {
10959
model: modelWithoutArtifactAndVpc,
11060
variantName: 'minimalServerlessVariant',
@@ -115,7 +65,7 @@ new sagemaker.EndpointConfig(stack, 'MinimalServerlessEndpointConfig', {
11565
});
11666

11767
// Test serverless endpoint configuration with boundary values
118-
new sagemaker.EndpointConfig(stack, 'BoundaryServerlessEndpointConfig', {
68+
const boundaryServerlessEndpointConfig = new sagemaker.EndpointConfig(stack, 'BoundaryServerlessEndpointConfig', {
11969
serverlessProductionVariant: {
12070
model: modelWithoutArtifactAndVpc,
12171
variantName: 'boundaryServerlessVariant',
@@ -125,6 +75,58 @@ new sagemaker.EndpointConfig(stack, 'BoundaryServerlessEndpointConfig', {
12575
},
12676
});
12777

128-
new IntegTest(app, 'integtest-endpointconfig', {
78+
const integ = new IntegTest(app, 'integtest-endpointconfig', {
12979
testCases: [stack],
13080
});
81+
82+
// Verify instance-based endpoint config
83+
integ.assertions.awsApiCall('SageMaker', 'describeEndpointConfig', {
84+
EndpointConfigName: endpointConfig.endpointConfigName,
85+
}).expect(ExpectedResult.objectLike({
86+
ProductionVariants: [
87+
{ VariantName: 'firstVariant', InstanceType: 'ml.m5.large' },
88+
{ VariantName: 'secondVariant' },
89+
{ VariantName: 'thirdVariant' },
90+
],
91+
}));
92+
93+
// Verify serverless endpoint config with all properties
94+
integ.assertions.awsApiCall('SageMaker', 'describeEndpointConfig', {
95+
EndpointConfigName: serverlessEndpointConfig.endpointConfigName,
96+
}).expect(ExpectedResult.objectLike({
97+
ProductionVariants: [{
98+
VariantName: 'serverlessVariant',
99+
ServerlessConfig: {
100+
MaxConcurrency: 10,
101+
MemorySizeInMB: 2048,
102+
ProvisionedConcurrency: 5,
103+
},
104+
}],
105+
}));
106+
107+
// Verify minimal serverless endpoint config
108+
integ.assertions.awsApiCall('SageMaker', 'describeEndpointConfig', {
109+
EndpointConfigName: minimalServerlessEndpointConfig.endpointConfigName,
110+
}).expect(ExpectedResult.objectLike({
111+
ProductionVariants: [{
112+
VariantName: 'minimalServerlessVariant',
113+
ServerlessConfig: {
114+
MaxConcurrency: 1,
115+
MemorySizeInMB: 1024,
116+
},
117+
}],
118+
}));
119+
120+
// Verify boundary serverless endpoint config
121+
integ.assertions.awsApiCall('SageMaker', 'describeEndpointConfig', {
122+
EndpointConfigName: boundaryServerlessEndpointConfig.endpointConfigName,
123+
}).expect(ExpectedResult.objectLike({
124+
ProductionVariants: [{
125+
VariantName: 'boundaryServerlessVariant',
126+
ServerlessConfig: {
127+
MaxConcurrency: 200,
128+
MemorySizeInMB: 6144,
129+
ProvisionedConcurrency: 200,
130+
},
131+
}],
132+
}));

0 commit comments

Comments
 (0)