Skip to content

fix: Validation args now use default framework_version for TensorFlow #1087

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
Oct 16, 2019
Merged

fix: Validation args now use default framework_version for TensorFlow #1087

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d505210
fix: Validation args now use default framework_version
Oct 12, 2019
f0bd539
Merge branch 'master' into fix/tf-framework-version
Oct 15, 2019
919b036
Merge branch 'master' into fix/tf-framework-version
Oct 16, 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 @@ -302,7 +302,7 @@ def __init__(
self._validate_args(
py_version=py_version,
script_mode=script_mode,
framework_version=framework_version,
framework_version=self.framework_version,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

We should probably pass in the self referenced version of all the passed parameters in order to keep it consistent. Is that okay with you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually... I'm wondering if it makes more sense to validate before defaulting. Since there is a chance that a user wants to use X, but due to this change it will default and pass when in theory it should have failed. Your thoughts?

Copy link
Contributor Author

@hyche hyche Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think I understand what you meant. Because the document says
framework_version (str): ..If not specified, this will default to 1.11.
That's why I made this change, so we should validate after defaulting right, otherwise if will throw exception on None value whereas it shouldn't ? However, I think there is better way to fix this, ie. Following this approach (https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/tensorflow/model.py#L56), which using not-None default arguments. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hyche, I agree with you.

As for the better way of fixing it, I think we can't default at the parameter level due to this method expecting a None:

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

Line 282 in d505210

if framework_version is None:

The original code change you provided works!

training_steps=training_steps,
evaluation_steps=evaluation_steps,
requirements_file=requirements_file,
Expand Down