feat(sagemaker): add support uncompressed model #30949

WinterYukky · 2024-07-25T15:43:46Z

Issue # (if applicable)

None.

Reason for this change

Currently, sagemaker.Model only support ModelDataUrl. ModelDataUrl must be gzip the model artifact.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-sagemaker-model-containerdefinition.html#cfn-sagemaker-model-containerdefinition-modeldataurl

LLM, which is popular these days, is large and takes longer to compress. For example, the 70B parameter model could take 2 hours or more to compress. SageMaker has a way to use the uncompressed model, and this PR is needed to support the uncompressed model.

The uncompressed model is also recommended in the official documentation.

When deploying ML models, one option is to archive and compress the model artifacts into a tar.gz format. Although this method works well for small models, compressing a large model artifact with hundreds of billions of parameters and then decompressing it on an endpoint can take a significant amount of time. For large model inference, we recommend that you deploy uncompressed ML model.
https://docs.aws.amazon.com/sagemaker/latest/dg/large-model-inference-uncompressed.html

Description of changes

Users can now specify the use of the uncompressed model by taking the model's compression type and S3 data type as options. When using an uncompressed model, use ModelDataSource instead of ModelDataUrl. Since ModelDataSource cannot be used in some workloads, the compression model uses ModelDataUrl as before.

Note

Currently you cannot use ModelDataSource in conjunction with SageMaker batch transform, SageMaker serverless endpoints, SageMaker multi-model endpoints, and SageMaker Marketplace.
https://docs.aws.amazon.com/sagemaker/latest/APIReference/API_ContainerDefinition.html#sagemaker-Type-ContainerDefinition-ModelDataSource

Also, asset bundling can now be used as a by-product of being able to use something other than tar.gz due to support for uncompressed models. Until now, no model artifacts with extensions other than tar.gz were permitted, but bundled assets are now allowed if they are not directories.

Since it is not possible to upload a directory or extract a zip with s3_asset.asset alone, the directory cannot continue to be used as an asset. (If there is a way to support directories, please let me know)

Description of how you validated changes

I've added an integ test with assertions that invoke the model endpoint and verify the response.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

Other

sagemaker-alpha module document is not aligned contributing guide recommendation, so also I fix the it.
https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#recommendations

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

WinterYukky · 2024-07-25T15:52:42Z

packages/@aws-cdk/aws-sagemaker-alpha/lib/model-data.ts

-    if (!path.toLowerCase().endsWith(ARTIFACT_EXTENSION)) {
-      throw new Error(`Asset must be a gzipped tar file with extension ${ARTIFACT_EXTENSION} (${this.path})`);
-    }


The asset's availability determination was moved after binding it as an asset.

WinterYukky · 2024-07-25T15:53:17Z

packages/@aws-cdk/aws-sagemaker-alpha/lib/model-data.ts

+    if (!this.asset.isFile) {
+      throw new Error(`Asset must be a file, if you want to use directory you can use 'ModelData.fromBucket()' with the 's3DataType' option to 'S3DataType.S3_PREFIX' and 'compressionType' option to 'CompressionType.NONE' (${this.path})`);
+    }


The bundled assets are now allowed if they are not directories.

WinterYukky · 2024-07-25T15:53:53Z

packages/@aws-cdk/aws-sagemaker-alpha/lib/model-data.ts

+      compressionType: this.asset.assetPath.toLowerCase().endsWith(COMPRESSED_ARTIFACT_EXTENSION)
+        ? CompressionType.GZIP
+        : CompressionType.NONE,
+      s3DataType: S3DataType.S3_OBJECT,


Uncompressed single files are also supported

…ithub.com/WinterYukky/aws-cdk into feat/sagemaker/support-uncompressed-model

Leo10Gama

Thanks for the contribution! This is a great start for this feature, but I have a few concerns with the level of abstraction we're providing, specifically with the two enums for CompressionType and S3DataType. We should generally avoid situations where we tell users "if you want to use this, you also have to set this variable that's somewhere else", since it leads to a lesser user experience. If there's some way to refactor this as one enum, I think that would be ideal.

Leo10Gama · 2024-10-31T18:07:19Z

packages/@aws-cdk/aws-sagemaker-alpha/lib/model-data.ts

+ * When you choose `CompressionType.GZIP` and `S3DataType.S3_OBJECT` then use `ModelDataUrl` property.
+ * Otherwise, use `ModelDataSource` property.


Reading this, I interpret it as telling the user to use the ModelDataUrl or ModelDataSource properties, but looking through the rest of the changes, it seems like this is something handled internally. Is there a reason the user needs to know about what's going on under the hood?

Leo10Gama · 2024-10-31T18:11:29Z

packages/@aws-cdk/aws-sagemaker-alpha/lib/model-data.ts

+   * If you choose `CompressionType.NONE` and choose `S3DataType.S3_PREFIX` as the value of `s3DataType`,
+   * S3 URI identifies a key name prefix, under which all objects represents the uncompressed ML model to deploy.
+   *
+   * If you choose `CompressionType.NONE`, then SageMaker will follow rules below when creating model data files
+   * under `/opt/ml/model` directory for use by your inference code:


The CompressionType and S3DataType enums seem to be used a lot in conjunction with each other, and when their behaviour changes this drastically between combinations, it makes me wonder if there is any way we can combine the two? Right now, for instance, there doesn't seem to be anything documented about what will happen if I combine CompressionType.GZIP and S3DataType.S3_PREFIX, if that's a valid combination.

Leo10Gama · 2024-10-31T18:12:53Z

packages/@aws-cdk/aws-sagemaker-alpha/lib/model-data.ts

+   * - Do not use any of the following as file names or directory names:
+   *   - An empty or blank string
+   *   - A string which contains null bytes
+   *   - A string longer than 255 bytes
+   *   - A single dot (.)
+   *   - A double dot (..)
+   * - Ambiguous file names will result in model deployment failure.
+   *    For example, if your uncompressed ML model consists of two S3 objects `s3://mybucket/model/weights` and `s3://mybucket/model/weights/part1`
+   *    and you specify `s3://mybucket/model/` as the value of S3 URI and `S3DataType.S3_PREFIX` as the value of `s3DataType`,
+   *    then it will result in name clash between `/opt/ml/model/weights` (a regular file) and `/opt/ml/model/weights/` (a directory).


This docstring feels unnecessarily lengthy, and the mention of what file/directory names doesn't seem relevant, at least for the enum. Is there anyway we can add a @see tag and link to the documentation instead for all this extra information?

Leo10Gama · 2024-10-31T18:23:07Z

packages/@aws-cdk/aws-sagemaker-alpha/lib/model.ts

+      modelDataSource: useModelDataSource ? {
+        s3DataSource: {
+          s3Uri: modelDataConfig.uri,
+          s3DataType: modelDataConfig.s3DataType!,
+          compressionType: modelDataConfig.compressionType!,
+        },
+      } : undefined,
+      modelDataUrl: !useModelDataSource ? modelDataConfig?.uri : undefined,


Would it be possible for both of these values to be set at the same time? And if so, what sort of behaviour would we expect?

aws-cdk-automation · 2024-11-22T00:11:44Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

Pull request has been modified.

codecov · 2024-11-22T16:19:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.77%. Comparing base (bf026bd) to head (351e729).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #30949   +/-   ##
=======================================
  Coverage   78.77%   78.77%           
=======================================
  Files         108      108           
  Lines        7163     7163           
  Branches     1320     1320           
=======================================
  Hits         5643     5643           
  Misses       1335     1335           
  Partials      185      185

Flag	Coverage Δ
suite.unit	`78.77% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.77% <ø> (ø)`

aws-cdk-automation · 2024-12-14T00:11:39Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2024-12-14T13:32:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 351e729
Result: FAILED
Build Logs (available for 30 days)

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

feat(sagemaker): add support uncompressed model

5af304a

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jul 25, 2024

aws-cdk-automation requested a review from a team July 25, 2024 15:43

github-actions bot added the p2 label Jul 25, 2024

fix: un-expect change

90cf701

WinterYukky commented Jul 25, 2024

View reviewed changes

WinterYukky added 3 commits July 26, 2024 01:00

Merge branch 'main' into feat/sagemaker/support-uncompressed-model

518bfbc

fix: missing import

69265e1

Merge branch 'feat/sagemaker/support-uncompressed-model' of https://g…

a4882a0

…ithub.com/WinterYukky/aws-cdk into feat/sagemaker/support-uncompressed-model

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 25, 2024

This was referenced Jul 29, 2024

Weekly PR metrics report #30970

Closed

Monthly PR metrics report #30999

Closed

Leo10Gama self-assigned this Oct 25, 2024

Leo10Gama previously requested changes Oct 31, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 31, 2024

Merge branch 'aws:main' into feat/sagemaker/support-uncompressed-model

88ec55f

Merge branch 'main' into feat/sagemaker/support-uncompressed-model

351e729

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sagemaker): add support uncompressed model #30949

feat(sagemaker): add support uncompressed model #30949

WinterYukky commented Jul 25, 2024 •

edited

Loading

WinterYukky Jul 25, 2024

WinterYukky Jul 25, 2024

WinterYukky Jul 25, 2024

Leo10Gama left a comment

Leo10Gama Oct 31, 2024

Leo10Gama Oct 31, 2024

Leo10Gama Oct 31, 2024

Leo10Gama Oct 31, 2024

aws-cdk-automation commented Nov 22, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

aws-cdk-automation commented Dec 14, 2024

aws-cdk-automation commented Dec 14, 2024

		* When you choose `CompressionType.GZIP` and `S3DataType.S3_OBJECT` then use `ModelDataUrl` property.
		* Otherwise, use `ModelDataSource` property.

feat(sagemaker): add support uncompressed model #30949

Are you sure you want to change the base?

feat(sagemaker): add support uncompressed model #30949

Conversation

WinterYukky commented Jul 25, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Other

WinterYukky Jul 25, 2024

Choose a reason for hiding this comment

WinterYukky Jul 25, 2024

Choose a reason for hiding this comment

WinterYukky Jul 25, 2024

Choose a reason for hiding this comment

Leo10Gama left a comment

Choose a reason for hiding this comment

Leo10Gama Oct 31, 2024

Choose a reason for hiding this comment

Leo10Gama Oct 31, 2024

Choose a reason for hiding this comment

Leo10Gama Oct 31, 2024

Choose a reason for hiding this comment

Leo10Gama Oct 31, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 22, 2024

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

aws-cdk-automation commented Dec 14, 2024

aws-cdk-automation commented Dec 14, 2024

AWS CodeBuild CI Report

WinterYukky commented Jul 25, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading