fix: preserve model_dir bool value #1954

chuyang-deng · 2020-10-15T00:41:58Z

Issue #, if available:
TensorFlow estimator's param model_dir will be changed to None if it was set to False by user after .hyperparameter() is called in https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/estimator.py#L1116-L1117 (.hyperparameters() called twice). It will later get set to a default s3 value when it's called the second time because its value is no longer False https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/tensorflow/estimator.py#L319.

Description of changes:
Adding a disable_model_dir param to TensorFlow class to make sure the boolean value is used as intended.
Corresponding change in sagemaker-tensorflow-training-toolkit: aws/sagemaker-tensorflow-training-toolkit#403

Testing done:
tox -e py38 -- tests/unit
tox -e py38 -- tests/component/test_tf_estimator.py

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 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 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 · 2020-10-15T00:49:56Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-15T01:17:58Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-15T01:22:11Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-15T01:50:54Z

AWS CodeBuild CI Report

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

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

icywang86rui · 2020-10-15T22:16:35Z

Can we fix it in https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/estimator.py#L1116-L1117
by assigning the result of hyperparamter() and eliminate the send call?

icywang86rui · 2020-10-15T22:25:18Z

or if you want to limit blast radius fix it here -

sagemaker-python-sdk/src/sagemaker/tensorflow/estimator.py

Lines 320 to 321 in 21ef053

    
           self.model_dir = self.model_dir or self._default_s3_path("model", mpi=mpi_enabled) 
        
           additional_hyperparameters["model_dir"] = self.model_dir

additional_hyperparameters["model_dir"] = self.model_dir or self._default_s3_path("model", mpi=mpi_enabled)

chuyang-deng · 2020-10-15T22:34:26Z

or if you want to limit blast radius fix it here -

sagemaker-python-sdk/src/sagemaker/tensorflow/estimator.py

Lines 320 to 321 in 21ef053

self.model_dir = self.model_dir or self._default_s3_path("model", mpi=mpi_enabled)

additional_hyperparameters["model_dir"] = self.model_dir

additional_hyperparameters["model_dir"] = self.model_dir or self._default_s3_path("model", mpi=mpi_enabled)

This will also result in setting additional_hyperparameters["model_dir"] to a default s3 uri if self.model_dir is None.

I will try with making changes here: https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/estimator.py#L1116-L1117 (I was hesitant at first because I was afraid it might break something else, but I will make sure it doesn't)

icywang86rui · 2020-10-15T22:44:09Z

or if you want to limit blast radius fix it here -

sagemaker-python-sdk/src/sagemaker/tensorflow/estimator.py

Lines 320 to 321 in 21ef053

self.model_dir = self.model_dir or self._default_s3_path("model", mpi=mpi_enabled)

additional_hyperparameters["model_dir"] = self.model_dir

additional_hyperparameters["model_dir"] = self.model_dir or self._default_s3_path("model", mpi=mpi_enabled)

This will also result in setting additional_hyperparameters["model_dir"] to a default s3 uri if self.model_dir is None.

I will try with making changes here: https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/estimator.py#L1116-L1117 (I was hesitant at first because I was afraid it might break something else, but I will make sure it doesn't)

you can make it explicit by adding another if statement

sagemaker-bot · 2020-10-16T04:06:11Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T04:30:21Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T04:33:24Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T04:38:24Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T17:34:02Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T18:03:11Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T18:24:04Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T18:28:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2020-10-16T19:36:16Z

AWS CodeBuild CI Report

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

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

metrizable · 2020-10-16T21:15:24Z

src/sagemaker/estimator.py

-            hyperparameters = {str(k): str(v) for (k, v) in estimator.hyperparameters().items()}
+        current_hyperparameters = estimator.hyperparameters()
+        if current_hyperparameters is not None:
+            hyperparameters = {str(k): str(v) for (k, v) in current_hyperparameters.items()}


you can remove the parentheses:

Suggested change

hyperparameters = {str(k): str(v) for (k, v) in current_hyperparameters.items()}

hyperparameters = {str(k): str(v) for k, v in current_hyperparameters.items()}

sagemaker-bot · 2020-10-16T21:59:09Z

AWS CodeBuild CI Report

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

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

* fix: preserve model_dir bool value * avoid calling hyperparaneters twince Co-authored-by: Chuyang Deng <[email protected]> Co-authored-by: icywang86rui <[email protected]>

fix: preserve model_dir bool value

854513c

chuyang-deng mentioned this pull request Oct 15, 2020

fix: add condition to avoid error when 'model_dir' is None aws/sagemaker-tensorflow-training-toolkit#403

Merged

Merge branch 'master' into model-dir-false

1453a12

avoid calling hyperparaneters twince

9c8d2d4

icywang86rui approved these changes Oct 16, 2020

View reviewed changes

metrizable approved these changes Oct 16, 2020

View reviewed changes

chuyang-deng merged commit a27d9d7 into aws:master Oct 16, 2020

	hyperparameters = {str(k): str(v) for (k, v) in current_hyperparameters.items()}
	hyperparameters = {str(k): str(v) for k, v in current_hyperparameters.items()}

fix: preserve model_dir bool value #1954

fix: preserve model_dir bool value #1954

Uh oh!

Conversation

chuyang-deng commented Oct 15, 2020 • 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 Oct 15, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 15, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 15, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 15, 2020

AWS CodeBuild CI Report

Uh oh!

icywang86rui commented Oct 15, 2020

Uh oh!

icywang86rui commented Oct 15, 2020

Uh oh!

chuyang-deng commented Oct 15, 2020

Uh oh!

icywang86rui commented Oct 15, 2020

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

metrizable Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Oct 16, 2020

AWS CodeBuild CI Report

Uh oh!

Uh oh!

chuyang-deng commented Oct 15, 2020 •

edited

Loading