feature: script mode for model class #2841

evakravi · 2022-01-10T23:00:39Z

Issue #, if available:

Description of changes:
This PR adds script mode support to the Model class.

This was basically done by adding the parameters source_dir, code_location, entry_point, container_log_level, dependencies, git_config to the Model class.

Testing done:
The changes to the Model class do not break any existing unit tests. In addition, new unit tests were added to simulate the script mode use case for the Model class, and confirm that the calls to sagemaker.deploy_endpoint(), sagemaker.create_model(), sagemaker.create_endpoint_configuration() and s3 are the same for the Model class and FrameworkModel class when both use script mode. In addition, this PR confirms that git_config can be used with the new Model class. Integration tests will be introduced in a subsequent PR.

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

sagemaker-bot · 2022-01-10T23:17:46Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-10T23:56:26Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T00:37:54Z

AWS CodeBuild CI Report

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

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

JGuinegagne

Same comment as #2834 on unit tests to cover (if applicable):

git_config
dependencies

JGuinegagne · 2022-01-11T15:25:45Z

src/sagemaker/model.py

        )

+    def _upload_code(self, key_prefix, repack=False):


i know you are only refactoring, but could you please add args and return typings?

JGuinegagne · 2022-01-11T15:27:28Z

src/sagemaker/model.py

        )

+    def _upload_code(self, key_prefix, repack=False):
+        """Placeholder Docstring"""


Would you be able to update the docstring?

codecov-commenter · 2022-01-11T16:28:30Z

Codecov Report

Merging #2841 (b7c9586) into master-jumpstart (00f23e6) will decrease coverage by 0.00%.
The diff coverage is 89.55%.

@@                 Coverage Diff                  @@
##           master-jumpstart    #2841      +/-   ##
====================================================
- Coverage             89.16%   89.16%   -0.01%     
====================================================
  Files                   185      185              
  Lines                 16043    16047       +4     
====================================================
+ Hits                  14305    14308       +3     
- Misses                 1738     1739       +1

Impacted Files	Coverage Δ
src/sagemaker/huggingface/model.py	`57.37% <0.00%> (ø)`
src/sagemaker/model.py	`88.92% <90.00%> (+0.13%)`	⬆️
src/sagemaker/chainer/model.py	`97.56% <100.00%> (ø)`
src/sagemaker/mxnet/model.py	`96.36% <100.00%> (ø)`
src/sagemaker/pytorch/model.py	`85.45% <100.00%> (ø)`
src/sagemaker/sklearn/model.py	`95.12% <100.00%> (ø)`
src/sagemaker/workflow/airflow.py	`91.49% <100.00%> (ø)`
src/sagemaker/xgboost/model.py	`97.43% <100.00%> (ø)`
src/sagemaker/workflow/parameters.py	`98.57% <0.00%> (-1.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00f23e6...b7c9586. Read the comment docs.

sagemaker-bot · 2022-01-11T16:35:56Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T17:13:16Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T17:14:01Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T17:26:28Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T17:47:05Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T17:53:40Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T17:58:14Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-11T18:30:55Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T20:15:10Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T20:55:44Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T21:29:24Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T21:54:17Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T22:47:46Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T22:48:32Z

AWS CodeBuild CI Report

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

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

JGuinegagne

non-blocking question: why moving **/test_model.py to **/model/test_model.py ?

sagemaker-bot · 2022-01-14T23:44:57Z

AWS CodeBuild CI Report

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

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

mufaddal-rohawala · 2022-01-18T20:20:39Z

src/sagemaker/model.py

+            code_location (str): Name of the S3 bucket where custom code is
+                uploaded (default: None). If not specified, default bucket
+                created by ``sagemaker.session.Session`` is used.
+            entry_point (str): Path (absolute or relative) to the Python source


need to add (default: None)

sagemaker-bot · 2022-01-18T20:59:24Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-18T21:31:06Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-18T21:41:48Z

AWS CodeBuild CI Report

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

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

shreyapandit · 2022-01-19T18:28:40Z

src/sagemaker/model.py

+        if self.source_dir or self.dependencies or self.entry_point or self.git_config:
+            if self.key_prefix or self.git_config:
+                self._upload_code(deploy_key_prefix, repack=False)
+            elif self.source_dir and self.entry_point:
+                self._upload_code(deploy_key_prefix, repack=True)
+            else:
+                self._upload_code(deploy_key_prefix, repack=False)
+            deploy_env.update(self._script_mode_env_vars())
        return sagemaker.container_def(


Can we concise this as:

if self.source_dir and self.entry_point: self._upload_code(deploy_key_prefix, repack=True) else: self._upload_code(deploy_key_prefix, repack=False)

and with that you can even use ternary to condense it into a single line further

shreyapandit · 2022-01-19T18:29:31Z

src/sagemaker/model.py

+        if repack and self.model_data is not None and self.entry_point is not None:
+            if isinstance(self.model_data, sagemaker.workflow.properties.Properties):
+                # model is not yet there, defer repacking to later during pipeline execution
+                return


This feels similar to elsewhere in the code, can we in future pull this into a common utility?

We have this logic only once in the code, though there's similar packaging code used for the Estimator classes.

feat: script mode for model class

613ba6a

evakravi marked this pull request as draft January 10, 2022 23:00

JGuinegagne requested changes Jan 11, 2022

View reviewed changes

change: update docstring for _upload_code

169d8d1

change: update _upload_code typing, add git test for model class

574b9a2

evakravi marked this pull request as ready for review January 11, 2022 17:27

Merge branch 'master-jumpstart' into feat/script_mode_for_model_class

03615ae

change: move unit tests

bb2b8d4

JGuinegagne approved these changes Jan 14, 2022

View reviewed changes

mufaddal-rohawala reviewed Jan 18, 2022

View reviewed changes

change: update docstring for Model class

b7c9586

mufaddal-rohawala approved these changes Jan 19, 2022

View reviewed changes

shreyapandit reviewed Jan 19, 2022

View reviewed changes

shreyapandit approved these changes Jan 19, 2022

View reviewed changes

mufaddal-rohawala changed the title ~~feat: script mode for model class~~ feature: script mode for model class Jan 19, 2022

mufaddal-rohawala merged commit 167b723 into aws:master-jumpstart Jan 19, 2022

feature: script mode for model class #2841

feature: script mode for model class #2841

Uh oh!

Conversation

evakravi commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

sagemaker-bot commented Jan 10, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 10, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

JGuinegagne left a comment

Choose a reason for hiding this comment

Uh oh!

JGuinegagne Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

JGuinegagne Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 11, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

JGuinegagne left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

evakravi commented Jan 10, 2022 •

edited

Loading

codecov-commenter commented Jan 11, 2022 •

edited

Loading