fix: make instance type fields as optional #3135

rahven14 · 2022-05-25T17:16:49Z

Issue #, if available:

Description of changes:

Make instance type fields as Optional in SDK, to be in sync with the API.

Testing done:

unit

Commands Used:

tox -e py39 tests/unit

tox -e py39 tests/unit/sagemaker/workflow/test_pipeline_session.py::test_pipeline_session_context_for_model_step_without_instance_types

tox -e py39 tests/unit/test_session.py::test_create_model_package_from_containers_without_instance_types

tox -e py39 tests/unit/test_estimator.py::test_register_default_image_without_instance_type_args

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-05-25T17:20:53Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-25T18:01:34Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-25T18:17:49Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-25T18:19:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-25T18:53:14Z

AWS CodeBuild CI Report

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

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

…rahven14/sagemaker-python-sdk into fix/make-instance-type-optional

sagemaker-bot · 2022-05-26T07:23:47Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T07:23:50Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T07:58:08Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T08:05:00Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T08:08:19Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T08:18:53Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T08:19:17Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T08:26:01Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-26T08:27:17Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T15:31:48Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T16:00:25Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T16:15:16Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T16:16:04Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2022-06-02T16:20:41Z

Codecov Report

Merging #3135 (e5b551b) into master (60723ed) will decrease coverage by 0.06%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #3135      +/-   ##
==========================================
- Coverage   89.69%   89.62%   -0.07%     
==========================================
  Files        1048      200     -848     
  Lines       87811    17391   -70420     
==========================================
- Hits        78763    15587   -63176     
+ Misses       9048     1804    -7244

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`90.90% <ø> (ø)`
src/sagemaker/huggingface/model.py	`79.10% <0.00%> (ø)`
src/sagemaker/model.py	`92.58% <ø> (ø)`
src/sagemaker/pytorch/model.py	`85.45% <0.00%> (ø)`
src/sagemaker/sklearn/model.py	`81.63% <0.00%> (ø)`
src/sagemaker/workflow/step_collections.py	`97.95% <ø> (ø)`
src/sagemaker/mxnet/model.py	`96.36% <100.00%> (ø)`
src/sagemaker/pipeline.py	`84.93% <100.00%> (ø)`
src/sagemaker/session.py	`70.89% <100.00%> (ø)`
src/sagemaker/tensorflow/model.py	`95.00% <100.00%> (ø)`
... and 848 more

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 60723ed...e5b551b. Read the comment docs.

sagemaker-bot · 2022-06-02T16:20:42Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T16:23:14Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T16:26:27Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T16:34:41Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T16:46:37Z

AWS CodeBuild CI Report

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

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

qidewenwhen · 2022-06-02T19:21:01Z

src/sagemaker/pipeline.py

@@ -313,7 +313,7 @@ def register(
            if model.model_data is None:
                raise ValueError("SageMaker Model Package cannot be created without model data.")
        if model_package_group_name is not None:
-            container_def = self.pipeline_container_def(inference_instances[0])
+            container_def = self.pipeline_container_def(inference_instances[0] if inference_instances else None)


Can you also update the instance_type argument in pipeline_container_def and make its default value is None?
https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/pipeline.py#L87

qidewenwhen · 2022-06-02T19:22:37Z

src/sagemaker/session.py

            raise ValueError(
-                "content_types, response_types, inference_inferences and transform_instances "
+                "content_types and response_types "
                "must be provided if containers is present."
            )


This thrown error was added by sreedes@. Did you check with her to see if she agree on this?

Discussed the same with Saumitra and got approval for this change

qidewenwhen · 2022-06-02T19:29:57Z

tests/unit/sagemaker/workflow/test_pipeline_session.py

+    assert pipeline_session_mock.context is None
+    assert create_step_args.create_model_request
+    assert not create_step_args.create_model_package_request
+    assert len(create_step_args.need_runtime_repack) == 1


Nit: seems that you only update the model.register() and this test should focus on your changes - "without instance_types". Thus I guess we don't need to test the model.create case here

qidewenwhen · 2022-06-02T19:34:23Z

tests/unit/sagemaker/workflow/test_pipeline_session.py

+    assert not pipeline_session_mock.context
+    assert not register_step_args.create_model_request
+    assert register_step_args.create_model_package_request
+    assert len(register_step_args.need_runtime_repack) == 0


Nit: same above, these validations should focus on your changes, which is how removing instance_type impacting the register_step_args.create_model_package_request

Please remove the validations of 157, 158 and 160. And elaborate a little bit on line 159 if possible

Changed as per request

…g parameter

sagemaker-bot · 2022-06-04T08:56:34Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-04T09:05:41Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-04T09:18:30Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-04T09:20:51Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-04T09:38:01Z

AWS CodeBuild CI Report

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

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

rahven14 and others added 2 commits May 25, 2022 22:35

change: Make Instance Type Fields as Optional

1470afb

Merge branch 'aws:master' into fix/make-instance-type-optional

b57ad62

rahven14 changed the title ~~Fix/make instance type optional~~ fix: make instance type fields as optional May 25, 2022

rahven14 and others added 3 commits May 26, 2022 12:49

fix: fix pylint errors

effd3ee

Merge branch 'aws:master' into fix/make-instance-type-optional

6dcfd45

Merge branch 'fix/make-instance-type-optional' of https://github.com/…

2c4fa5d

…rahven14/sagemaker-python-sdk into fix/make-instance-type-optional

Merge branch 'master' into fix/make-instance-type-optional

54a8463

fixed black-format issues

bf054de

qidewenwhen reviewed Jun 2, 2022

View reviewed changes

addressed PR comments for UT's and added default value None to missin…

e5b551b

…g parameter

qidewenwhen approved these changes Jun 6, 2022

View reviewed changes

mufaddal-rohawala requested review from mufaddal-rohawala and shreyapandit June 7, 2022 16:52

shreyapandit approved these changes Jun 7, 2022

View reviewed changes

mufaddal-rohawala approved these changes Jun 8, 2022

View reviewed changes

mufaddal-rohawala merged commit 90ede1b into aws:master Jun 8, 2022

jerrypeng7773 pushed a commit to jerrypeng7773/sagemaker-python-sdk that referenced this pull request Aug 8, 2022

fix: make instance type fields as optional (aws#3135)

4d27ff4

fix: make instance type fields as optional #3135

fix: make instance type fields as optional #3135

Conversation

rahven14 commented May 25, 2022 • edited Loading

Merge Checklist

General

Tests

sagemaker-bot commented May 25, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 25, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 25, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 25, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 25, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

codecov-commenter commented Jun 2, 2022 • edited Loading

Codecov Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagemaker-bot commented Jun 4, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 4, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 4, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 4, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 4, 2022

AWS CodeBuild CI Report

rahven14 commented May 25, 2022 •

edited

Loading

codecov-commenter commented Jun 2, 2022 •

edited

Loading