Skip to content
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.

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

what-if: upgrade what-if source to be tf 2.0 compatible #2331

Merged
merged 4 commits into from
Jun 24, 2019

Conversation

stephanwlee
Copy link
Contributor

@stephanwlee stephanwlee commented Jun 7, 2019

The utility was using TF v1 only API.

Where necessary, we now use tf.compat.v1 instead of tf.
Also ran the migration script to add keywords to the arguments to some methods.

Report from the migration script:

INFO line 673:23: Added keywords to args of function 'tf.shape'
INFO line 713:17: Added keywords to args of function 'tf.cond'
INFO line 718:17: Added keywords to args of function 'tf.while_loop'
INFO line 749:8: Added keywords to args of function 'tf.parse_example'                              
INFO line 749:8: Renamed 'tf.parse_example' to 'tf.io.parse_example'                                                                                                                                    
TensorFlow 2.0 Upgrade Script       
-----------------------------
Converted 1 files             
Detected 0 issues that require attention                    
--------------------------------------------------------------------------------

Edit: ran the migration script to the interactive_inference source tree and it made minor changes to notebook/base.py.

Fixes #2330.

WANT_LGTM=nfelt

@stephanwlee stephanwlee requested a review from jameswex June 7, 2019 16:47
@stephanwlee stephanwlee changed the title Upgrade inference_utils to be tf 2.0 compatible what-if: upgrade inference_utils to be tf 2.0 compatible Jun 7, 2019
@stephanwlee stephanwlee changed the title what-if: upgrade inference_utils to be tf 2.0 compatible what-if: upgrade what-if source to be tf 2.0 compatible Jun 7, 2019
@stephanwlee stephanwlee requested a review from nfelt June 10, 2019 14:55
@@ -711,13 +711,13 @@ def loop_body(i, encoded_images, images):
resized_image = tf.image.resize(image, thumbnail_dims)
expanded_image = tf.expand_dims(resized_image, 0)
images = tf.cond(
tf.equal(i, 0), lambda: expanded_image,
lambda: tf.concat([images, expanded_image], 0))
pred=tf.equal(i, 0), true_fn=lambda: expanded_image,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the keyword arguments are actually necessary here since we're only using args that have stayed in the same position (but the migration script would automatically change them to kwargs since other arguments have moved).

Same goes for tf.while_loop below and tf.shape(input=...) above, and I think also for tf.io.parse_example() although passing keyword args there seems fair since the argument semantics are pretty non-obvious, but for these ones I think the argument semantics are clear (and more idiomatic) without names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes were made by the migration script. I'd revert the changes back but I want to know why TF migration script makes un-idiomatic changes. Is it that bad?

Copy link
Contributor

Choose a reason for hiding this comment

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

The migration script migrates the arguments of any function with a changed signature to be keyword arguments, even if the function isn't called with arguments that were affected by the change. I think it's just simpler that way since then you don't have to do as much analysis of the callsite and also the signature changes (which could be argument renames or reorders) - each migration can be done as a self-contained step rather than worrying about preserving the details of which args were called with names or without names.

I agree it would be nicer if it did produce idiomatic TF 2.0 code, but this is part of why manual review of the changes is recommended anyway - the goal is for the code it produces to work but it won't necessarily be idiomatic.

@stephanwlee stephanwlee requested a review from nfelt June 11, 2019 04:02
@@ -711,13 +711,13 @@ def loop_body(i, encoded_images, images):
resized_image = tf.image.resize(image, thumbnail_dims)
expanded_image = tf.expand_dims(resized_image, 0)
images = tf.cond(
tf.equal(i, 0), lambda: expanded_image,
lambda: tf.concat([images, expanded_image], 0))
pred=tf.equal(i, 0), true_fn=lambda: expanded_image,
Copy link
Contributor

Choose a reason for hiding this comment

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

The migration script migrates the arguments of any function with a changed signature to be keyword arguments, even if the function isn't called with arguments that were affected by the change. I think it's just simpler that way since then you don't have to do as much analysis of the callsite and also the signature changes (which could be argument renames or reorders) - each migration can be done as a self-contained step rather than worrying about preserving the details of which args were called with names or without names.

I agree it would be nicer if it did produce idiomatic TF 2.0 code, but this is part of why manual review of the changes is recommended anyway - the goal is for the code it produces to work but it won't necessarily be idiomatic.

@stephanwlee stephanwlee merged commit 861be4e into tensorflow:master Jun 24, 2019
@stephanwlee stephanwlee deleted the wit-comp branch June 24, 2019 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What If Tool for Tensorflow 2.0
3 participants