fix: Validation args now use default framework_version for TensorFlow #1087
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
hyche
force-pushed
the
fix/tf-framework-version
branch
from
6edc62d to
d505210
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
hyche
changed the title
ChoiByungWook
left a comment
ChoiByungWook
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR!
| py_version=py_version, | ||
| script_mode=script_mode, | ||
| framework_version=framework_version, | ||
| framework_version=self.framework_version, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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:
The original code change you provided works!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
ChoiByungWook
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
3 participants
Which should be passed modified variable.
Description of changes:
framework_version is passed from passed argument but not modified self.framework_version, which leads to false validation, although it shouldn't
Merge Checklist
Put an
xin 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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.