fix: Fix Tensorflow default model_dir generation when output_path is pipeline variable #3146

qidewenwhen · 2022-05-28T02:38:07Z

Issue #, if available: #3142

Description of changes:
Fix Tensorflow default model_dir generation when output_path is pipeline variable. Originally, if model_dir is None, output_path would be used to generate a default model_dir. However, this won't work if output_path is a PipelineVariable. This PR updates to use the default bucket to replace output_path in this case.

Testing done: unit tests and integ 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 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-28T03:01:45Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2022-05-28T03:01:45Z

Codecov Report

Merging #3146 (06c1370) into master (4acbdb0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3146   +/-   ##
=======================================
  Coverage   89.62%   89.62%           
=======================================
  Files         200      200           
  Lines       17387    17391    +4     
=======================================
+ Hits        15583    15587    +4     
  Misses       1804     1804

Impacted Files	Coverage Δ
src/sagemaker/tensorflow/estimator.py	`95.61% <100.00%> (+0.15%)`	⬆️

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 4acbdb0...06c1370. Read the comment docs.

sagemaker-bot · 2022-05-28T03:13:36Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-28T03:23:55Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-28T03:26:39Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-28T03:44:01Z

AWS CodeBuild CI Report

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

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

…pipeline variable

sagemaker-bot · 2022-06-01T21:56:14Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-01T21:58:56Z

AWS CodeBuild CI Report

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

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

navinsoni · 2022-06-01T22:09:59Z

src/sagemaker/tensorflow/estimator.py

@@ -378,6 +379,9 @@ def _default_s3_path(self, directory, mpi=False):
        if mpi:
            return "/opt/ml/model"
        if self._current_job_name:
+            if is_pipeline_variable(self.output_path):
+                output_path = "s3://{}".format(self.sagemaker_session.default_bucket())


what happens here if customer is using custom S3 bucket

Thanks for reviewing!
This update is within the _default_s3_path which means it's generating a default path for the model_dir output if user does not specify it. Thus I think that's fine.

If users hope to use custom s3 bucket they should explicitly assign a s3 path to model_dir field of TensorFlow

sagemaker-bot · 2022-06-01T22:17:58Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-01T22:21:37Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-01T22:41:17Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-06-02T00:10:29Z

AWS CodeBuild CI Report

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

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

…pipeline variable (aws#3146)

qidewenwhen requested a review from jerrypeng7773 May 28, 2022 02:38

jerrypeng7773 approved these changes May 31, 2022

View reviewed changes

mufaddal-rohawala approved these changes Jun 1, 2022

View reviewed changes

qidewenwhen mentioned this pull request Jun 1, 2022

Not able to have the value of the output_path in TensorFlow estimator as pipeline parameter #3149

Closed

fix: Fix Tensorflow default model_dir generation when output_path is …

06c1370

…pipeline variable

qidewenwhen force-pushed the tf branch from e21b4cc to 06c1370 Compare June 1, 2022 21:34

navinsoni reviewed Jun 1, 2022

View reviewed changes

navinsoni approved these changes Jun 1, 2022

View reviewed changes

navinsoni merged commit 62eee2a into aws:master Jun 2, 2022

Lokiiiiii pushed a commit to Lokiiiiii/sagemaker-python-sdk that referenced this pull request Jun 2, 2022

fix: Fix Tensorflow default model_dir generation when output_path is …

406052b

…pipeline variable (aws#3146)

qidewenwhen deleted the tf branch January 25, 2024 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix Tensorflow default model_dir generation when output_path is pipeline variable #3146

fix: Fix Tensorflow default model_dir generation when output_path is pipeline variable #3146

qidewenwhen commented May 28, 2022

sagemaker-bot commented May 28, 2022

codecov-commenter commented May 28, 2022 •

edited

Loading

sagemaker-bot commented May 28, 2022

sagemaker-bot commented May 28, 2022

sagemaker-bot commented May 28, 2022

sagemaker-bot commented May 28, 2022

sagemaker-bot commented Jun 1, 2022

sagemaker-bot commented Jun 1, 2022

navinsoni Jun 1, 2022

qidewenwhen Jun 1, 2022

sagemaker-bot commented Jun 1, 2022

sagemaker-bot commented Jun 1, 2022

sagemaker-bot commented Jun 1, 2022

sagemaker-bot commented Jun 2, 2022

fix: Fix Tensorflow default model_dir generation when output_path is pipeline variable #3146

fix: Fix Tensorflow default model_dir generation when output_path is pipeline variable #3146

Conversation

qidewenwhen commented May 28, 2022

Merge Checklist

General

Tests

sagemaker-bot commented May 28, 2022

AWS CodeBuild CI Report

codecov-commenter commented May 28, 2022 • edited Loading

Codecov Report

sagemaker-bot commented May 28, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 28, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 28, 2022

AWS CodeBuild CI Report

sagemaker-bot commented May 28, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 1, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 1, 2022

AWS CodeBuild CI Report

navinsoni Jun 1, 2022

Choose a reason for hiding this comment

qidewenwhen Jun 1, 2022

Choose a reason for hiding this comment

sagemaker-bot commented Jun 1, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 1, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 1, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jun 2, 2022

AWS CodeBuild CI Report

codecov-commenter commented May 28, 2022 •

edited

Loading