fix: account for EI and version-based ECR repo naming in serving_image_uri() #1273

laurenyu · 2020-01-30T01:17:42Z

Issue #, if available:

Description of changes:

fix: Append serving to model framework name #1247 didn't account for the fact that for each framework version with a "framework-serving" ECR repo, earlier versions of the framework didn't have the suffix.
In addition, the serving_image_uri() method that was originally added in fix: Use serving_image_uri for Airflow #1245 didn't originally have an option for EI because we don't have parity for Airflow yet, but there was the opportunity to remove duplicated code by reusing the new method in each model class, so I added the accelerator option for that.
Lastly, I changed the Airflow unit test that was changed in fix: Append serving to model framework name #1247 to use the latest MXNet image URI format rather than change it back to the old one.

Testing done:

tox -e pylint,flake8,black-format
unit tests

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 used the commit message format described in CONTRIBUTING
I have used the regional endpoint when creating S3 and/or STS clients (if appropriate)
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 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.

…e_uri()

sagemaker-bot · 2020-01-30T01:24:42Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T01:26:07Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T01:31:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T01:31:40Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T01:41:44Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T01:48:01Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T01:51:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T01:53:25Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T02:01:11Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T02:07:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

src/sagemaker/pytorch/model.py

knakad · 2020-01-30T17:27:53Z

src/sagemaker/pytorch/model.py

-                self.py_version,
-                accelerator_type=accelerator_type,
+            deploy_image = self.serving_image_uri(
+                region_name, instance_type, accelerator_type=accelerator_type


[Minor] Keyword/Named arguments make kittens happy.

talked offline - I don't see a significant benefit of doing region_name=region_name over region_name

knakad · 2020-01-30T17:28:36Z

src/sagemaker/pytorch/model.py

        """Create a URI for the serving image.

        Args:
            region_name (str): AWS region where the image is uploaded.
            instance_type (str): SageMaker instance type. Used to determine device type
                (cpu/gpu/family-specific optimized).
+            accelerator_type (str): The Elastic Inference accelerator type to
+                deploy to the instance for loading and making inferences to the
+                model. Currently unsupported with PyTorch.


Same comment as above. What happens if this is passed in? Is the img not found?

src/sagemaker/tensorflow/model.py

ajaykarpur · 2020-01-30T17:42:48Z

src/sagemaker/mxnet/model.py

-        return fw_utils.create_image_uri(
+        framework_name = self.__framework_name__
+        if self._is_mms_version():
+            framework_name += "-serving"


String join is more efficient than string concatenation. For more details, see:

https://stackoverflow.com/questions/3055477/how-slow-is-pythons-string-concatenation-vs-str-join

https://waymoot.org/home/python_string/

Suggested change

framework_name += "-serving"

framework_name = "-".join([framework_name, "serving"])

talked offline - we'll go with format strings as a compromise between performance and readability, since the performance gain is negligible in this case

an aside - fwiw, a tuple would be more efficient than a list because it's immutable

ajaykarpur · 2020-01-30T17:43:12Z

src/sagemaker/pytorch/model.py

-        return fw_utils.create_image_uri(
+        framework_name = self.__framework_name__
+        if self._is_mms_version():
+            framework_name += "-serving"


String join is more efficient than string concatenation. For more details, see:

https://stackoverflow.com/questions/3055477/how-slow-is-pythons-string-concatenation-vs-str-join

https://waymoot.org/home/python_string/

Suggested change

framework_name += "-serving"

framework_name = "-".join([framework_name, "serving"])

sagemaker-bot · 2020-01-30T17:47:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T17:54:31Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T18:07:32Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T18:13:24Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T18:52:44Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T18:59:57Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T19:11:21Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-01-30T19:20:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

…old model versions aws#1273 (aws#1301)

laurenyu added 2 commits January 29, 2020 17:11

fix: account for EI and version-based ECR repo naming in serving_imag…

105e46a

…e_uri()

Merge branch 'master' into serving-uri-accelerator-type

b3ecc28

laurenyu requested a review from ajaykarpur January 30, 2020 01:20

Merge branch 'master' into serving-uri-accelerator-type

ca7b716

knakad previously approved these changes Jan 30, 2020

View reviewed changes

Merge branch 'master' into serving-uri-accelerator-type

aa88c1d

ajaykarpur previously approved these changes Jan 30, 2020

View reviewed changes

address PR comments

b4e2559

laurenyu dismissed stale reviews from ajaykarpur and knakad via b4e2559 January 30, 2020 18:44

laurenyu requested a review from knakad January 30, 2020 18:45

knakad approved these changes Jan 30, 2020

View reviewed changes

laurenyu merged commit ab52225 into aws:master Jan 30, 2020

laurenyu deleted the serving-uri-accelerator-type branch January 30, 2020 20:31

whittech1 pushed a commit to whittech1/sagemaker-python-sdk that referenced this pull request Nov 14, 2023

feat: Enhance jumpstart logging for wildcard version identifiers and …

f6a1692

…old model versions aws#1273 (aws#1301)

	framework_name += "-serving"
	framework_name = "-".join([framework_name, "serving"])

fix: account for EI and version-based ECR repo naming in serving_image_uri() #1273

fix: account for EI and version-based ECR repo naming in serving_image_uri() #1273

Uh oh!

Conversation

laurenyu commented Jan 30, 2020

Merge Checklist

General

Tests

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

Uh oh!

knakad Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

laurenyu Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

knakad Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajaykarpur Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

laurenyu Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

ajaykarpur Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 30, 2020

AWS CodeBuild CI Report