fix: intelligent defaults for volume size, JS Estimator image uri region, Predictor str method #3870

evakravi · 2023-05-18T21:27:13Z

Description of changes:

Fixes intelligent defaults for when a KMS key used with an unsupported instance type
Fixes bug with JumpStartEstimator where the region wasn't being used with the image_uri
Add __str__ method to base Predictor class
Fix volume_size_supported for local mode
Add ignore_intelligent_defaults to SessionSettings

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.

…ion, Predictor str method

…olume-kms

sagemaker-bot · 2023-05-18T21:35:39Z

AWS CodeBuild CI Report

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

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

JGuinegagne · 2023-05-18T21:31:28Z

src/sagemaker/base_predictor.py


+    def __str__(self) -> str:
+        """Overriding str(*) method to make more human-readable."""
+        return stringify_object(self)


this looks to me like a good improvement, but please double check with SDK team if that's considered backward incompatible please. The alternative to be to write a different serialization function.

Agree this would be a good improvement, I dont see any negative implications of this.

JGuinegagne · 2023-05-18T21:35:19Z

src/sagemaker/estimator.py

+                ]
+            )
+            else volume_kms_key
        )


nit: I find ternary hard to read in this context, could you do:

if <condition>: self.volume_kms_key = resolve_value_from_config(...) else: self.volume_kms_key = volume_kms_key

More importantly, from a security/reasonable expection POV: should we let this code throw if any of the instance in the group does support KMS, but some do not?

If we silently remove it as you do here, the result is that there will be instances with EBS volumes out there that aren't encrypted with the default KMS key provided by admins. I don't think that's desirable.

I think this is a valid concern with respect to instance_groups, lets involve the instance_groups feature team to weigh in on this.

JGuinegagne · 2023-05-18T21:46:56Z

src/sagemaker/session.py

            request["KmsKeyId"] = new_kms_key or existing_endpoint_config_desc.get("KmsKeyId")
-        if KMS_KEY_ID not in request:
+
+        supports_kms = all(


same comment here please, I would vote for letting it fail in that case.

JGuinegagne · 2023-05-18T21:49:18Z

src/sagemaker/session.py

            str: The name of the created ``Endpoint``.
        """
+
+        supports_kms = all(


same comment here please

JGuinegagne · 2023-05-18T21:52:55Z

tests/unit/test_estimator.py

+    assert estimator.output_kms_key == expected_kms_key_id
+    assert estimator.volume_kms_key is None
+    assert estimator.security_group_ids == expected_security_groups
+    assert estimator.subnets == expected_subnets


please add a test case where the instance group has types that support attaching a volume and types that do not support it.

JGuinegagne · 2023-05-18T21:53:21Z

tests/unit/test_session.py

+    assert actual_train_args["OutputDataConfig"] == {
+        "S3OutputPath": S3_OUTPUT,
+        "KmsKeyId": expected_kms_key_id,
+    }


same comment here please

sagemaker-bot · 2023-05-18T22:37:41Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-18T22:44:32Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-18T22:57:36Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-18T23:01:50Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2023-05-18T23:04:40Z

Codecov Report

Merging #3870 (9f0533f) into master (abd94ab) will decrease coverage by 0.71%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #3870      +/-   ##
==========================================
- Coverage   90.08%   89.37%   -0.71%     
==========================================
  Files        1148      269     -879     
  Lines      106048    26234   -79814     
==========================================
- Hits        95534    23447   -72087     
+ Misses      10514     2787    -7727

Impacted Files	Coverage Δ
src/sagemaker/experiments/run.py	`99.14% <ø> (ø)`
src/sagemaker/instance_types.py	`89.47% <ø> (ø)`
src/sagemaker/jumpstart/factory/estimator.py	`98.42% <ø> (ø)`
src/sagemaker/base_predictor.py	`88.42% <66.66%> (ø)`
src/sagemaker/estimator.py	`91.27% <88.88%> (ø)`
src/sagemaker/jumpstart/artifacts/kwargs.py	`89.47% <100.00%> (ø)`
src/sagemaker/jumpstart/estimator.py	`98.21% <100.00%> (ø)`
src/sagemaker/jumpstart/model.py	`97.56% <100.00%> (ø)`
src/sagemaker/pipeline.py	`86.25% <100.00%> (ø)`
src/sagemaker/session.py	`78.68% <100.00%> (ø)`
... and 2 more

... and 1405 files with indirect coverage changes

sagemaker-bot · 2023-05-19T00:24:15Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T00:29:48Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T00:46:33Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T00:52:22Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T01:58:27Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T02:08:34Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T02:16:09Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T02:20:59Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-19T02:27:29Z

AWS CodeBuild CI Report

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

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

…olume-kms

sagemaker-bot · 2023-05-19T13:59:08Z

AWS CodeBuild CI Report

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

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

JGuinegagne · 2023-05-19T13:54:31Z

src/sagemaker/estimator.py

        else:
            raise ValueError(f"Bad value for instance type: '{instance_type}'")

-        self.volume_kms_key = (


thanks for making the change, could you add an inline comment here to explain what you are doing?

Most readers won't be familiar with instance_group and the details of which hardware supports attaching an EBS volume that can be encrypted and which do not.

Also nit: could you rename the variable use_volume_kms_config please?

JGuinegagne · 2023-05-19T13:59:10Z

tests/unit/test_session.py

            "DefaultS3Bucket"
        ]
    )



optional: how about renaming this variable expected_volume_kms_key_id for better readability?

JGuinegagne · 2023-05-19T14:00:12Z

tests/unit/test_session.py

+        ],
+        KmsKeyId=expected_kms_key_id,  # from config
+        Tags=expected_tags,  # from config
+        AsyncInferenceConfig={"OutputConfig": {"KmsKeyId": expected_inference_kms_key_id}},


same comment please

sagemaker-bot · 2023-05-25T14:03:05Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-25T14:20:14Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-25T16:03:40Z

AWS CodeBuild CI Report

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

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

…olume-kms

sagemaker-bot · 2023-05-30T19:44:20Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-30T19:50:06Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-30T20:05:06Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-30T20:16:20Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-30T22:01:42Z

AWS CodeBuild CI Report

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

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

mufaddal-rohawala

/bot run slow-tests, notebook-tests

sagemaker-bot · 2023-05-30T22:26:34Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-30T23:22:39Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-31T20:35:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-31T20:39:19Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-31T20:54:48Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-31T21:06:05Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-31T21:19:04Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-01T12:48:31Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-01T12:51:37Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-01T13:10:00Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-01T13:18:36Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-01T13:21:34Z

AWS CodeBuild CI Report

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

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

jerrypeng7773 · 2023-06-30T18:46:23Z

src/sagemaker/utils.py

+    try:
+
+        # local mode does not support volume size
+        if instance_type.startswith("local"):


will need to add another check or is_pipeline_variable(instance_type), otherwise, this breaks pipeline cx.

fix: intelligent defaults for volume size, JS Estimator image uri reg…

3456684

…ion, Predictor str method

evakravi requested a review from a team as a code owner May 18, 2023 21:27

evakravi requested review from font44 and removed request for a team May 18, 2023 21:27

Merge remote-tracking branch 'origin' into fix/intelligent-defaults-v…

d42cad6

…olume-kms

evakravi marked this pull request as draft May 18, 2023 21:29

knikure requested review from rubanh and removed request for font44 May 18, 2023 21:49

chore: assume kms support if 1 instance in group supports kms

8c379c1

JGuinegagne requested changes May 18, 2023

View reviewed changes

chore: address PR comments

fc4096b

fix: failing unit tests

55e93d9

Merge remote-tracking branch 'origin' into fix/intelligent-defaults-v…

fe4092a

…olume-kms

JGuinegagne reviewed May 19, 2023

View reviewed changes

evakravi added 2 commits May 30, 2023 18:59

chore: remove ignore sagemaker config flag

b6e5f8c

Merge remote-tracking branch 'origin' into fix/intelligent-defaults-v…

7709a87

…olume-kms

rubanh approved these changes May 30, 2023

View reviewed changes

mufaddal-rohawala reviewed May 30, 2023

View reviewed changes

Merge branch 'master' into fix/intelligent-defaults-volume-kms

c19fc9d

Merge branch 'master' into fix/intelligent-defaults-volume-kms

9f0533f

knikure merged commit e981daa into aws:master Jun 1, 2023

pviolette3 mentioned this pull request Jun 7, 2023

Passing instance type as a ParameterString to PyTorch estimator in 2.163.0 crashes #3917

Closed

jerrypeng7773 reviewed Jun 30, 2023

View reviewed changes

qidewenwhen mentioned this pull request Aug 10, 2023

ValueError(f"Bad value for instance type: '{instance_type}'") #3993

Closed

fix: intelligent defaults for volume size, JS Estimator image uri region, Predictor str method #3870

fix: intelligent defaults for volume size, JS Estimator image uri region, Predictor str method #3870

Uh oh!

Conversation

evakravi commented May 18, 2023 • 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 May 18, 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!

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!

sagemaker-bot commented May 18, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 18, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 18, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 18, 2023

AWS CodeBuild CI Report

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented May 19, 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!

Choose a reason for hiding this comment

evakravi commented May 18, 2023 •

edited

Loading

codecov-commenter commented May 18, 2023 •

edited

Loading