Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 19, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f209e6e
fix: use correct TF version in empty framework_version warning
laurenyu Apr 18, 2019
42c7f30
Merge branch 'master' into tf-latest-version-var
laurenyu Apr 18, 2019
4a723b5
Merge branch 'master' into tf-latest-version-var
laurenyu Apr 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sagemaker/tensorflow/estimator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link

@Nishanksingla Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@laurenyu laurenyu Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@Nishanksingla Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok Thanks for the explanation.

self.framework_version = framework_version or TF_VERSION
Copy link

@Nishanksingla Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still you haven't changed TF_VERSION to LATEST_VERSION

Copy link

@Nishanksingla Nishanksingla Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@laurenyu laurenyu Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


super(TensorFlow, self).__init__(image_name=image_name, **kwargs)
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test_tf_estimator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ def test_empty_framework_version(warning, sagemaker_session):
framework_version=None)

assert estimator.framework_version == defaults.TF_VERSION
warning.assert_called_with(defaults.TF_VERSION, defaults.TF_VERSION)
warning.assert_called_with(defaults.TF_VERSION, estimator.LATEST_VERSION)


def _deprecated_args_msg(args):
Expand Down