fix: use correct TF version in empty framework_version warning #756

laurenyu · 2019-04-18T15:42:05Z

Description of changes:
Addresses #754 (LATEST_VERSION wasn't being used in the TensorFlow estimator)

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.

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

icywang86rui

ship!

jesterhazy · 2019-04-18T17:29:24Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

Nishanksingla · 2019-04-18T17:41:12Z

src/sagemaker/tensorflow/estimator.py

@@ -229,7 +229,7 @@ def __init__(self, training_steps=None, evaluation_steps=None, checkpoint_path=N
            **kwargs: Additional kwargs passed to the Framework constructor.
        """
        if framework_version is None:
-            LOGGER.warning(fw.empty_framework_version_warning(TF_VERSION, TF_VERSION))
+            LOGGER.warning(fw.empty_framework_version_warning(TF_VERSION, self.LATEST_VERSION))


self.LATEST_VERSION looks wrong to me as LATEST_VERSION is not class variable, it is defined outside the init.
It should be just LATEST_VERSION

it's defined in the class TensorFlow:

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

Lines 170 to 174 in 42c7f30

class TensorFlow(Framework):

"""Handle end-to-end training and deployment of user-provided TensorFlow code."""

__framework_name__ = 'tensorflow'

LATEST_VERSION = '1.12'

LATEST_VERSION (without self) would work only if it is defined outside of the TensorFlow class:

>>> class A: ... FOO = 1 ... def __init__(self): ... print(FOO) ... >>> A() Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 4, in __init__ NameError: name 'FOO' is not defined

Ok Thanks for the explanation.

Nishanksingla · 2019-04-18T17:59:08Z

src/sagemaker/tensorflow/estimator.py

@@ -229,7 +229,7 @@ def __init__(self, training_steps=None, evaluation_steps=None, checkpoint_path=N
            **kwargs: Additional kwargs passed to the Framework constructor.
        """
        if framework_version is None:
-            LOGGER.warning(fw.empty_framework_version_warning(TF_VERSION, TF_VERSION))
+            LOGGER.warning(fw.empty_framework_version_warning(TF_VERSION, self.LATEST_VERSION))
        self.framework_version = framework_version or TF_VERSION


Still you haven't changed TF_VERSION to LATEST_VERSION

The initial reported issue is that TF_VERSION is set '1.11' and LATEST_VERSION is set '1.12'. TF_VERSION should be replaced by LATEST_VERSION. Or TF_VERSION should be updated to 1.12

I understand that the transition from having an updating default value to requiring framework_version be explicitly defined could be better, and I apologize for the inconvenience. There's more explanation in #754 around this, and there'll be a separate PR for the documentation changes proposed in #754.

jesterhazy · 2019-04-18T19:28:34Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

jesterhazy · 2019-04-18T23:42:10Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

jesterhazy · 2019-04-19T02:50:31Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

jesterhazy · 2019-04-19T16:59:58Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

laurenyu · 2019-04-19T17:01:21Z

will make a separate PR to address the doc concerns raised in #754

* change: Clean up test resources to remove model files * fix: Change experiment enums to upper case * change: Upgrade boto3 and update test to validate mixed case name * fix: Update as per latest botocore release and backend change Co-authored-by: Dewen Qi <qidewen@amazon.com>

* feature: Add experiment plus Run class (#691) * feature: Add Experiment helper classes (#646) * feature: Add Experiment helper classes feature: Add helper class _RunEnvironment * change: Change sleep retry to backoff retry for get TC * minor fixes in backoff retry Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add helper classes and methods for Run class (#660) * feature: Add helper classes and methods for Run class * Add Parent class to address comment * fix docstyle check * Add arg docstrings in _helper Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add Experiment Run class (#651) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Add integ tests for Run (#673) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Update run log metric to use MetricsManager (#678) * Update run.log_metric to use _MetricsManager * fix several metrics issues * Add doc strings to metrics.py Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> * change: Simplify exp plus integ test configuration (#694) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: add RunName to expeirment_config (#696) * change: Update Run init and add Run load and _RunContext (#707) * change: Update Run init and add Run load Add exp name and run group name to load and address comments * Address nit comments Co-authored-by: Dewen Qi <qidewen@amazon.com> * fix: Fix run name uniqueness issue (#730) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Update integ tests for Exp Plus M1 changes (#741) Co-authored-by: Dewen Qi <qidewen@amazon.com> * add metrics client to session object (#745) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * change: Add integ test for using Run in Transform Job (#749) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Add async metrics sink (#739) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * use metrics client provided by session (#754) * fix flaky metrics test (#753) * change: Change Run.init and Run.load to constructor and module method respectively (#752) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add latest metric service model (#757) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * fix: lowercase run name (#767) * Change: Minimize use of lower case tc name (#769) * change: Clean up test resources to remove model files (#756) * change: Clean up test resources to remove model files * fix: Change experiment enums to upper case * change: Upgrade boto3 and update test to validate mixed case name * fix: Update as per latest botocore release and backend change Co-authored-by: Dewen Qi <qidewen@amazon.com> * lowercase trial component name (#776) * change: Expose sagemaker experiment doc strings * fix: Fix exp name mixed case in issue Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Yifei Zhu <66866419+yzhu0@users.noreply.github.com>

* feature: Add experiment plus Run class (aws#691) * feature: Add Experiment helper classes (aws#646) * feature: Add Experiment helper classes feature: Add helper class _RunEnvironment * change: Change sleep retry to backoff retry for get TC * minor fixes in backoff retry Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add helper classes and methods for Run class (aws#660) * feature: Add helper classes and methods for Run class * Add Parent class to address comment * fix docstyle check * Add arg docstrings in _helper Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add Experiment Run class (aws#651) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Add integ tests for Run (aws#673) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Update run log metric to use MetricsManager (aws#678) * Update run.log_metric to use _MetricsManager * fix several metrics issues * Add doc strings to metrics.py Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> * change: Simplify exp plus integ test configuration (aws#694) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: add RunName to expeirment_config (aws#696) * change: Update Run init and add Run load and _RunContext (aws#707) * change: Update Run init and add Run load Add exp name and run group name to load and address comments * Address nit comments Co-authored-by: Dewen Qi <qidewen@amazon.com> * fix: Fix run name uniqueness issue (aws#730) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Update integ tests for Exp Plus M1 changes (aws#741) Co-authored-by: Dewen Qi <qidewen@amazon.com> * add metrics client to session object (aws#745) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * change: Add integ test for using Run in Transform Job (aws#749) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Add async metrics sink (aws#739) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * use metrics client provided by session (aws#754) * fix flaky metrics test (aws#753) * change: Change Run.init and Run.load to constructor and module method respectively (aws#752) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add latest metric service model (aws#757) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * fix: lowercase run name (aws#767) * Change: Minimize use of lower case tc name (aws#769) * change: Clean up test resources to remove model files (aws#756) * change: Clean up test resources to remove model files * fix: Change experiment enums to upper case * change: Upgrade boto3 and update test to validate mixed case name * fix: Update as per latest botocore release and backend change Co-authored-by: Dewen Qi <qidewen@amazon.com> * lowercase trial component name (aws#776) * change: Expose sagemaker experiment doc strings * fix: Fix exp name mixed case in issue Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Yifei Zhu <66866419+yzhu0@users.noreply.github.com>

* feature: Add experiment plus Run class (#691) * feature: Add Experiment helper classes (#646) * feature: Add Experiment helper classes feature: Add helper class _RunEnvironment * change: Change sleep retry to backoff retry for get TC * minor fixes in backoff retry Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add helper classes and methods for Run class (#660) * feature: Add helper classes and methods for Run class * Add Parent class to address comment * fix docstyle check * Add arg docstrings in _helper Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add Experiment Run class (#651) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Add integ tests for Run (#673) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Update run log metric to use MetricsManager (#678) * Update run.log_metric to use _MetricsManager * fix several metrics issues * Add doc strings to metrics.py Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> * change: Simplify exp plus integ test configuration (#694) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: add RunName to expeirment_config (#696) * change: Update Run init and add Run load and _RunContext (#707) * change: Update Run init and add Run load Add exp name and run group name to load and address comments * Address nit comments Co-authored-by: Dewen Qi <qidewen@amazon.com> * fix: Fix run name uniqueness issue (#730) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Update integ tests for Exp Plus M1 changes (#741) Co-authored-by: Dewen Qi <qidewen@amazon.com> * add metrics client to session object (#745) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * change: Add integ test for using Run in Transform Job (#749) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Add async metrics sink (#739) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * use metrics client provided by session (#754) * fix flaky metrics test (#753) * change: Change Run.init and Run.load to constructor and module method respectively (#752) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add latest metric service model (#757) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * fix: lowercase run name (#767) * Change: Minimize use of lower case tc name (#769) * change: Clean up test resources to remove model files (#756) * change: Clean up test resources to remove model files * fix: Change experiment enums to upper case * change: Upgrade boto3 and update test to validate mixed case name * fix: Update as per latest botocore release and backend change Co-authored-by: Dewen Qi <qidewen@amazon.com> * lowercase trial component name (#776) * change: Expose sagemaker experiment doc strings * fix: Fix exp name mixed case in issue Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Yifei Zhu <66866419+yzhu0@users.noreply.github.com>

* feature: Add experiment plus Run class (aws#691) * feature: Add Experiment helper classes (aws#646) * feature: Add Experiment helper classes feature: Add helper class _RunEnvironment * change: Change sleep retry to backoff retry for get TC * minor fixes in backoff retry Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add helper classes and methods for Run class (aws#660) * feature: Add helper classes and methods for Run class * Add Parent class to address comment * fix docstyle check * Add arg docstrings in _helper Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add Experiment Run class (aws#651) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Add integ tests for Run (aws#673) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Update run log metric to use MetricsManager (aws#678) * Update run.log_metric to use _MetricsManager * fix several metrics issues * Add doc strings to metrics.py Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> * change: Simplify exp plus integ test configuration (aws#694) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: add RunName to expeirment_config (aws#696) * change: Update Run init and add Run load and _RunContext (aws#707) * change: Update Run init and add Run load Add exp name and run group name to load and address comments * Address nit comments Co-authored-by: Dewen Qi <qidewen@amazon.com> * fix: Fix run name uniqueness issue (aws#730) Co-authored-by: Dewen Qi <qidewen@amazon.com> * change: Update integ tests for Exp Plus M1 changes (aws#741) Co-authored-by: Dewen Qi <qidewen@amazon.com> * add metrics client to session object (aws#745) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * change: Add integ test for using Run in Transform Job (aws#749) Co-authored-by: Dewen Qi <qidewen@amazon.com> * Add async metrics sink (aws#739) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * use metrics client provided by session (aws#754) * fix flaky metrics test (aws#753) * change: Change Run.init and Run.load to constructor and module method respectively (aws#752) Co-authored-by: Dewen Qi <qidewen@amazon.com> * feature: Add latest metric service model (aws#757) Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: qidewenwhen <32910701+qidewenwhen@users.noreply.github.com> * fix: lowercase run name (aws#767) * Change: Minimize use of lower case tc name (aws#769) * change: Clean up test resources to remove model files (aws#756) * change: Clean up test resources to remove model files * fix: Change experiment enums to upper case * change: Upgrade boto3 and update test to validate mixed case name * fix: Update as per latest botocore release and backend change Co-authored-by: Dewen Qi <qidewen@amazon.com> * lowercase trial component name (aws#776) * change: Expose sagemaker experiment doc strings * fix: Fix exp name mixed case in issue Co-authored-by: Dewen Qi <qidewen@amazon.com> Co-authored-by: Dana Benson <danabens@amazon.com> Co-authored-by: Dana Benson <31262102+danabens@users.noreply.github.com> Co-authored-by: Yifei Zhu <66866419+yzhu0@users.noreply.github.com>

fix: use correct TF version in empty framework_version warning

f209e6e

icywang86rui approved these changes Apr 18, 2019

View reviewed changes

laurenyu mentioned this pull request Apr 18, 2019

Unused variable LATEST_VERSION in TensorFlow estimator #754

Closed

Merge branch 'master' into tf-latest-version-var

42c7f30

Nishanksingla reviewed Apr 18, 2019

View reviewed changes

Merge branch 'master' into tf-latest-version-var

4a723b5

laurenyu merged commit fd10a25 into aws:master Apr 19, 2019

laurenyu deleted the tf-latest-version-var branch April 19, 2019 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: use correct TF version in empty framework_version warning #756

fix: use correct TF version in empty framework_version warning #756

Uh oh!

laurenyu commented Apr 18, 2019

Uh oh!

icywang86rui left a comment

Uh oh!

jesterhazy commented Apr 18, 2019

Uh oh!

Nishanksingla Apr 18, 2019

Uh oh!

laurenyu Apr 18, 2019

Uh oh!

Nishanksingla Apr 18, 2019

Uh oh!

Nishanksingla Apr 18, 2019

Uh oh!

Nishanksingla Apr 18, 2019

Uh oh!

laurenyu Apr 18, 2019

Uh oh!

jesterhazy commented Apr 18, 2019

Uh oh!

jesterhazy commented Apr 18, 2019

Uh oh!

jesterhazy commented Apr 19, 2019

Uh oh!

jesterhazy commented Apr 19, 2019

Uh oh!

laurenyu commented Apr 19, 2019

Uh oh!

Uh oh!

	class TensorFlow(Framework):
	"""Handle end-to-end training and deployment of user-provided TensorFlow code."""

	__framework_name__ = 'tensorflow'
	LATEST_VERSION = '1.12'

fix: use correct TF version in empty framework_version warning #756

fix: use correct TF version in empty framework_version warning #756

Uh oh!

Conversation

laurenyu commented Apr 18, 2019

Merge Checklist

Uh oh!

icywang86rui left a comment

Choose a reason for hiding this comment

Uh oh!

jesterhazy commented Apr 18, 2019

AWS CodeBuild CI Report

Uh oh!

Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

laurenyu Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

laurenyu Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

jesterhazy commented Apr 18, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Apr 18, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Apr 19, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Apr 19, 2019

AWS CodeBuild CI Report

Uh oh!

laurenyu commented Apr 19, 2019

Uh oh!

Uh oh!