feat: Add deployment support for deployment recommendations #3695

jinpengqi · 2023-03-06T13:00:48Z

Issue #, if available:

Description of changes:

Testing done:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the Python SDK team
I used the commit message format described in CONTRIBUTING
I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
I have checked that my tests are not configured for a specific region or account (if appropriate)
I have used unique_name_from_base to create resource names in integ tests (if appropriate)

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

gwang111 · 2023-03-06T17:48:14Z

Can we clarify something?

Should instant recommendations be built into right_size()? Customer should be able to call right_size(instant_recommendations = True) or smth. Then within right_size we just call create and describe model to get the instant recommendation and store it to a model variable self.instant_recommendation i think.

if so, we could also consider consolidating our multiple model variables into one called self.right_size_results or smth

self.right_size_results = {
    inference_recommendations_results = ...
    inference_recommendations = ...
    instant_recommendations = ...
}

jinpengqi · 2023-03-07T13:00:01Z

Good point. For model name passed in case, since we'll create model for customer so we could check the results and store in self.instant_recommendations. But we might also need to inform customer who is invoking IR job with modelpkgarn that self.instant_recommendations is null since some customers might just create modelpkg directly.
From customer perspective, It should be extra info(optional to check)when invoking right_size since

they dont necessarily need to call right_size() for instant recommendation results,
when customer would like to call right_size() it usually means customer tries to invoke default inference recommendation jobs

gwang111 · 2023-03-08T01:40:58Z

got it. So we could have the option in right_size() but the main intention is to do instant recommendation directly from the deploy() method from recommendation_id?

jbarz1 · 2023-03-08T04:34:54Z

src/sagemaker/inference_recommender/inference_recommender_mixin.py

+        except sage_client.exceptions.ResourceNotFound:
+            pass
+        try:
+            model_res = sage_client.describe_model(ModelName=recommendation_job_name)


we don't need to make this call if describe_inference_recommendations_job works right?

May be we can create three private methods to modularize the code since this method is getting big.

def _get_model_recommendation_id(self): # get recommendation id from model def _get_right_size_recommendation_id(self): # get recommendation id from right size job def _get_recommendation_id(self): try: _get_right_size_recommendation_id(self) except: _get_model_recommendation_id(self) def _update_params_for_recommendation_id(...): try: recommendation_id = _get_recommendation_id(self) except: raise ValueError("Inference Recommendation id is not valid")

as we discussed before, what if customer's JobName and ModelName are the same, eg thanos/abcd1234 and thanos/abcd5678, then we need to find out it's inference or model recommendation. So go through two lists(inference_recommendations and model_recommendations) to get the recommendation is needed. So here I am thinking we need to loop through two lists anyway

good point, will create separate methods

we should only call describemodel if the recommendation id is not in the right_size recommendations.

Otherwise it is a wasted api call

jbarz1 · 2023-03-08T04:44:54Z

src/sagemaker/inference_recommender/inference_recommender_mixin.py

+                None,
+            )
+        if inference_recommendation is None and instant_recommendation is None:
+            raise ValueError("Inference Recommendation id does not exist")


id is invalid

ack, will modify

jbarz1 · 2023-03-08T04:46:40Z

src/sagemaker/inference_recommender/inference_recommender_mixin.py

+            raise ValueError("Inference Recommendation id does not exist")
+
+        # Update params beased on instant recommendation
+        if instant_recommendation:


could we not call it "instant"?

maybe use the names "right_size_recommendation" and "model_recommendation"?

Because that indicates the source of the recommendation

makes sense, will rename them

jinpengqi · 2023-03-08T08:12:22Z

got it. So we could have the option in right_size() but the main intention is to do instant recommendation directly from the deploy() method from recommendation_id?

I think we need to confirm if we want customer to use right_size to get the instant recommendations experience. Since today I believe the experience is obtained through create_model. And model.deploy() should be called when customer already inspect the recommendations results no matter its inference recommendation or instant recommendation so I think we don't leverage deploy to do instant recommendations.

Here is I think we could modify:
we set the fields instant_recommendations in right_size result but only for modelName passed in case, since we will create the temp model for customer, the instant_recommendations result should be good to have to be exposed. And we could instruct customers in notebook for this inspection too

@jbarz1 what do you think?

src/sagemaker/inference_recommender/inference_recommender_mixin.py

knikure

/bot run all

by mistake

knikure

/bot run all

sagemaker-bot · 2023-03-08T20:32:10Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: bfed49f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-03-08T20:36:37Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: bfed49f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

codecov-commenter · 2023-03-08T20:36:54Z

Codecov Report

Merging #3695 (e9af472) into master (9e5dca6) will decrease coverage by 0.78%.
The diff coverage is 88.09%.

@@            Coverage Diff             @@
##           master    #3695      +/-   ##
==========================================
- Coverage   89.79%   89.01%   -0.78%     
==========================================
  Files         980      230     -750     
  Lines       92208    22839   -69369     
==========================================
- Hits        82796    20330   -62466     
+ Misses       9412     2509    -6903

Impacted Files	Coverage Δ
...ference_recommender/inference_recommender_mixin.py	`91.00% <88.09%> (ø)`

... and 1209 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sagemaker-bot · 2023-03-08T20:40:22Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: bfed49f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-03-08T20:41:16Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: bfed49f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-03-08T22:46:49Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: bfed49f
Result: FAILED
Build Logs (available for 30 days)

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

…npengqi/sagemaker-python-sdk into instant-recommendation-deploy

knikure

/bot run all

sagemaker-bot · 2023-03-14T00:15:23Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: e9af472
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-03-14T00:25:40Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: e9af472
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-03-14T00:33:09Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: e9af472
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-03-14T00:35:01Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: e9af472
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-03-14T02:38:23Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: e9af472
Result: FAILED
Build Logs (available for 30 days)

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

jbarz1 · 2023-03-16T08:12:01Z

src/sagemaker/inference_recommender/inference_recommender_mixin.py

+        if model_recommendation:
+            if initial_instance_count is None:
+                raise ValueError(
+                    "Please specify initial_instance_count with model recommendation id"


prefer same style as messages in model.py

Must specify recommendation id and instance count.

thanks, modified in next rev

jbarz1 · 2023-03-16T08:16:26Z

src/sagemaker/inference_recommender/inference_recommender_mixin.py

            raise ValueError(
-                "inference_recommendation_id does not exist in InferenceRecommendations list"
+                "Please either do not specify instance_type and initial_instance_count"


similar to this

instance type and intial instance count are mutually exclusive with recommendation id

modified in next rev

jbarz1 · 2023-03-16T08:22:10Z

src/sagemaker/inference_recommender/inference_recommender_mixin.py

@@ -442,8 +446,10 @@ def _update_params_for_recommendation_id(
                self.model_data = compilation_res["ModelArtifacts"]["S3ModelArtifacts"]
                self.image_uri = compilation_res["InferenceImage"]

-        instance_type = recommendation["EndpointConfiguration"]["InstanceType"]
-        initial_instance_count = recommendation["EndpointConfiguration"]["InitialInstanceCount"]
+        instance_type = right_size_recommendation["EndpointConfiguration"]["InstanceType"]


what if right_size_recommendation is also None? Won't this fail ?

This case has been caught by here.
So _get_recommendation will ensure either right_size_recommendation or model_recommendation exists, nevertheless it will throw exception for invalid recommendation id

…npengqi/sagemaker-python-sdk into instant-recommendation-deploy

gwang111 · 2023-06-08T23:35:01Z

pushed commits on top of this. closing

Add deployment support for instant recommendations

9c6bc33

jinpengqi requested a review from a team as a code owner March 6, 2023 13:00

jinpengqi requested review from vikas0203 and removed request for a team March 6, 2023 13:00

jbarz1 reviewed Mar 8, 2023

View reviewed changes

jinpengqi added 2 commits March 8, 2023 20:03

Refactor get_recommendation

1179dc0

Merge branch 'master' into instant-recommendation-deploy

bfed49f

jbarz1 reviewed Mar 8, 2023

View reviewed changes

src/sagemaker/inference_recommender/inference_recommender_mixin.py Show resolved Hide resolved

knikure previously approved these changes Mar 8, 2023

View reviewed changes

knikure reviewed Mar 8, 2023

View reviewed changes

jinpengqi added 2 commits March 9, 2023 16:35

Modify to check recommendations job res first.

0918810

Merge branch 'instant-recommendation-deploy' of https://github.com/ji…

e9af472

…npengqi/sagemaker-python-sdk into instant-recommendation-deploy

jinpengqi changed the title ~~Add deployment support for instant recommendations~~ Add deployment support for deployment recommendations Mar 9, 2023

jinpengqi requested review from jbarz1 and removed request for vikas0203 March 9, 2023 08:38

knikure self-assigned this Mar 13, 2023

knikure reviewed Mar 13, 2023

View reviewed changes

knikure added the do-not-merge Use when PR needs to be marked as do not merge label Mar 14, 2023

jbarz1 reviewed Mar 16, 2023

View reviewed changes

jinpengqi added 3 commits March 20, 2023 15:54

Merge branch 'master' into instant-recommendation-deploy

e39ec4b

Update exception msg

f0e633e

Merge branch 'instant-recommendation-deploy' of https://github.com/ji…

81c052f

…npengqi/sagemaker-python-sdk into instant-recommendation-deploy

knikure changed the title ~~Add deployment support for deployment recommendations~~ feat: Add deployment support for deployment recommendations Jun 2, 2023

gwang111 mentioned this pull request Jun 2, 2023

feat: Add support for Deployment Recommendation ID in model.deploy() #3897

Closed

9 tasks

knikure removed the do-not-merge Use when PR needs to be marked as do not merge label Jun 5, 2023

gwang111 closed this Jun 8, 2023

gwang111 mentioned this pull request Jun 9, 2023

feat: Add support for Deployment Recommendation ID in model.deploy(). No tagging support #3920

Merged

9 tasks

gwang111 reopened this Jun 9, 2023

gwang111 closed this Jun 9, 2023

feat: Add deployment support for deployment recommendations #3695

feat: Add deployment support for deployment recommendations #3695

Uh oh!

Conversation

jinpengqi commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

gwang111 commented Mar 6, 2023

Uh oh!

jinpengqi commented Mar 7, 2023

Uh oh!

gwang111 commented Mar 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinpengqi commented Mar 8, 2023

Uh oh!

Uh oh!

knikure left a comment

Choose a reason for hiding this comment

Uh oh!

knikure left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Mar 8, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Mar 8, 2023

AWS CodeBuild CI Report

Uh oh!

codecov-commenter commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sagemaker-bot commented Mar 8, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Mar 8, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Mar 8, 2023

AWS CodeBuild CI Report

Uh oh!

knikure left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Mar 14, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Mar 14, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Mar 14, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Mar 14, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Mar 14, 2023

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinpengqi commented Mar 6, 2023 •

edited

Loading

codecov-commenter commented Mar 8, 2023 •

edited

Loading