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

make "smart_cond" api public and reusable #13954

Merged
merged 18 commits into from Feb 19, 2018

Conversation

@GeorgyZhou
Contributor

GeorgyZhou commented Oct 24, 2017

Fix #13903

Move the implementation of "smart_cond" and "constant_value" from tensorflow/python/layers/utils to tensorflow/python/ops/control_flow_ops and add corresponding test and expose smart_cond to tensorflow/python/ops/standard_ops.

@martinwicke should I implement the API in this way or not? As I think ops should be lower level API than layers and smart_cond should be a kind of ops. It's my first time to contribute to TensorFlow. Please feel free to correct me if there's any problem with the design and codes.

Tips, I closed the former PR #13938 and create a new since there's some signature/info problem with that PR.

@tensorflow-jenkins

This comment has been minimized.

Collaborator

tensorflow-jenkins commented Oct 24, 2017

Can one of the admins verify this patch?

@googlebot googlebot added the cla: yes label Oct 24, 2017

@netheril96

I think smart_cond should have the same naming scheme as cond. To elaborate, true_fn and false_fn rather than fn1 and fn2.

@GeorgyZhou

This comment has been minimized.

Contributor

GeorgyZhou commented Oct 25, 2017

@netheril96 Good point, I changed the naming schema. By the way @drpngx do you think we should remove the fn1 fn2 parameters in cond API? They seems redundant.

@benoitsteiner benoitsteiner requested a review from ebrevdo Oct 27, 2017

@benoitsteiner benoitsteiner self-assigned this Oct 27, 2017

@@ -195,19 +195,7 @@ def smart_cond(pred, fn1, fn2, name=None):
Raises:

This comment has been minimized.

@ebrevdo

ebrevdo Oct 28, 2017

Contributor

can't you just do:

smart_cond = control_flow_ops.smart_cond

?

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Oct 28, 2017

Contributor

I did consider that before, but I think current way could make the user/developer access the function doc more easily. I also referred to some other wrapper codes in the project such as https://github.com/tensorflow/tensorflow/blob/master/tensorflow/contrib/tpu/python/tpu/tpu.py#L499.

This comment has been minimized.

@ebrevdo

ebrevdo Oct 28, 2017

Contributor

Shouldn't the doc be the same, regardless of whether you copy it over or just use a function alias? The docstring copy is a DRY violation

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Oct 28, 2017

Contributor

I got it. Thanks for pointing out. I have submitted a commit to change to function alias assignment.

@@ -223,12 +211,4 @@ def constant_value(pred):
Raises:
TypeError: If `pred` is not a Variable, Tensor or bool.

This comment has been minimized.

@ebrevdo

ebrevdo Oct 28, 2017

Contributor

ditto

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Oct 28, 2017

Contributor

The same reason above.

pred_value = pred
elif isinstance(pred, variables.Variable):
pred_value = None
elif isinstance(pred, ops.Tensor):

This comment has been minimized.

@ebrevdo

ebrevdo Oct 28, 2017

Contributor

call this smart_constant_value and add it to tf.contrib.framework?

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Oct 28, 2017

Contributor

Yeah, I agree with you that the name is better and will submit a commit later.
I also considered whether to put this function in core or not and finally I found that the function was already used as internal interface in the core module before, I mean it's in tensorflow/python/layers/utils.py and used by some other modules like python/layers/normalization.py. What I did here should just make it public and reusable, so I didn't move it out of the core framework.

This comment has been minimized.

@ebrevdo

ebrevdo Oct 28, 2017

Contributor

It can continue to sit here, but for the public API expose it in contrib. The core API has much stricter behavior requirements and functions cannot be easily modified once exposed in core.

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Oct 28, 2017

Contributor

I get to know what you mean and I agree. I tried to submit a commit for that.

@@ -47,6 +47,7 @@
from tensorflow.python.ops.control_flow_ops import tuple
from tensorflow.python.ops.control_flow_ops import cond
from tensorflow.python.ops.control_flow_ops import case
from tensorflow.python.ops.control_flow_ops import smart_cond

This comment has been minimized.

@ebrevdo

ebrevdo Oct 28, 2017

Contributor

can you add this to tf.contrib.framework instead of to the core api?

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Oct 28, 2017

Contributor

Sure, I will import this file in tf.contrib.framework and submit a commit later

@drpngx

This comment has been minimized.

Member

drpngx commented Oct 31, 2017

Jenkins, test this please.

@drpngx drpngx added the kokoro:run label Oct 31, 2017

@kokoro-team kokoro-team removed the kokoro:run label Oct 31, 2017

@drpngx

This comment has been minimized.

Member

drpngx commented Oct 31, 2017

The XLA tests are failing because of this:

FAIL: //tensorflow/compiler/tests:conv3d_test_gpu (shard 5 of 5) (see /var/lib/jenkins/workspace/tensorflow-pull-requests-xla/bazel-ci_build-cache/.cache/bazel/_bazel_jenkins/eab0d61a99b6696edb3d2aff87b585e8/execroot/org_tensorflow/bazel-out/local_linux-py3-opt/testlogs/tensorflow/compiler/tests/conv3d_test_gpu/shard_5_of_5/test.log).
INFO: From Testing //tensorflow/compiler/tests:conv3d_test_gpu (shard 5 of 5):
==================== Test output for //tensorflow/compiler/tests:conv3d_test_gpu (shard 5 of 5):
Running test tensorflow/compiler/tests/conv3d_test_gpu --test_device=XLA_GPU --types=DT_FLOAT,DT_DOUBLE,DT_INT32,DT_INT64,DT_BOOL,DT_COMPLEX64 on GPU 4
Traceback (most recent call last):
  File "/var/lib/jenkins/workspace/tensorflow-pull-requests-xla/bazel-ci_build-cache/.cache/bazel/_bazel_jenkins/eab0d61a99b6696edb3d2aff87b585e8/execroot/org_tensorflow/bazel-out/local_linux-py3-opt/bin/tensorflow/compiler/tests/conv3d_test_gpu.runfiles/org_tensorflow/tensorflow/compiler/tests/conv3d_test.py", line 24, in <module>
    from tensorflow.compiler.tests.xla_test import XLATestCase
  File "/var/lib/jenkins/workspace/tensorflow-pull-requests-xla/bazel-ci_build-cache/.cache/bazel/_bazel_jenkins/eab0d61a99b6696edb3d2aff87b585e8/execroot/org_tensorflow/bazel-out/local_linux-py3-opt/bin/tensorflow/compiler/tests/conv3d_test_gpu.runfiles/org_tensorflow/tensorflow/compiler/tests/xla_test.py", line 35, in <module>
    from tensorflow.python.ops import variables
  File "/var/lib/jenkins/workspace/tensorflow-pull-requests-xla/bazel-ci_build-cache/.cache/bazel/_bazel_jenkins/eab0d61a99b6696edb3d2aff87b585e8/execroot/org_tensorflow/bazel-out/local_linux-py3-opt/bin/tensorflow/compiler/tests/conv3d_test_gpu.runfiles/org_tensorflow/tensorflow/python/ops/variables.py", line 27, in <module>
    from tensorflow.python.ops import control_flow_ops
  File "/var/lib/jenkins/workspace/tensorflow-pull-requests-xla/bazel-ci_build-cache/.cache/bazel/_bazel_jenkins/eab0d61a99b6696edb3d2aff87b585e8/execroot/org_tensorflow/bazel-out/local_linux-py3-opt/bin/tensorflow/compiler/tests/conv3d_test_gpu.runfiles/org_tensorflow/tensorflow/python/ops/control_flow_ops.py", line 75, in <module>
    from tensorflow.python.ops import variables
ImportError: cannot import name 'variables'

Some missing dependency?

GeorgyZhou added some commits Nov 5, 2017

@GeorgyZhou

This comment has been minimized.

Contributor

GeorgyZhou commented Nov 5, 2017

@drpngx The problem is that variables already import control_flow_ops and control_flow_ops cannot import variables. I already resolved it without loop dependencies. And It's my first pull request to TensorFlow and can you please tell me how I can test them? just comment Jenkins, test this please. and does it work from my comment?

@drpngx

This comment has been minimized.

Member

drpngx commented Nov 8, 2017

Jenkins, test this please.

@drpngx

This comment has been minimized.

Member

drpngx commented Nov 8, 2017

Someone from tensorflow needs to trigger the builds, sorry you can't do it yourself.

@drpngx drpngx added the kokoro:run label Nov 8, 2017

@kokoro-team kokoro-team removed the kokoro:run label Nov 8, 2017

@drpngx

This comment has been minimized.

Member

drpngx commented Nov 8, 2017

Same sycl issue, let's try again.

@rmlarsen

This comment has been minimized.

Member

rmlarsen commented Jan 24, 2018

@sguada please take another look at this.

@tensorflowbutler

This comment has been minimized.

Member

tensorflowbutler commented Feb 8, 2018

Nagging Assignee: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@sguada

sguada approved these changes Feb 9, 2018

@sguada

This comment has been minimized.

Member

sguada commented Feb 9, 2018

Sync and fix the failing tests

@martinwicke

This comment has been minimized.

Member

martinwicke commented Feb 15, 2018

@GeorgyZhou can you fix the failing test? constant_value is apparently used in contexts which require the current behavior.

@martinwicke

This comment has been minimized.

Member

martinwicke commented Feb 15, 2018

Here's the log:

File "/tmp/botexec/bazel-out/k8-opt/bin/tensorflow/python/keras/backend_test.runfiles/org_tensorflow/tensorflow/python/layers/normalization.py", line 544, in call
training_value = utils.constant_value(training)
File "/tmp/botexec/bazel-out/k8-opt/bin/tensorflow/python/keras/backend_test.runfiles/org_tensorflow/tensorflow/python/layers/utils.py", line 220, in constant_value
return control_flow_ops.smart_constant_value(pred)
File "/tmp/botexec/bazel-out/k8-opt/bin/tensorflow/python/keras/backend_test.runfiles/org_tensorflow/tensorflow/python/ops/control_flow_ops.py", line 2103, in smart_constant_value
raise TypeError('pred must be a Tensor or a Python bool.')
TypeError: pred must be a Tensor or a Python bool.

@GeorgyZhou

This comment has been minimized.

Contributor

GeorgyZhou commented Feb 15, 2018

Sure. I will resubmit soon today

"""
# Allow integer booleans.
if pred == 0:

This comment has been minimized.

@martinwicke

martinwicke Feb 16, 2018

Member

Should this be after the Variable check? There's a danger you'll get back a boolean Tensor if pred is a Variable?

This comment has been minimized.

@martinwicke

martinwicke Feb 16, 2018

Member

Or alternatively, should we check for type(pred) is int before we do this?

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Feb 16, 2018

Contributor

This is the original code during the review of the PR. I can do that as well

This comment has been minimized.

@GeorgyZhou

GeorgyZhou Feb 16, 2018

Contributor

Yeah I already did that. This PR took much more time than I thought before

@GeorgyZhou

This comment has been minimized.

Contributor

GeorgyZhou commented Feb 16, 2018

Thanks for lint correction! @martinwicke

@GeorgyZhou

This comment has been minimized.

Contributor

GeorgyZhou commented Feb 19, 2018

Anybody can merge it or can review it? @drpngx @martinwicke

@drpngx drpngx merged commit a4f1478 into tensorflow:master Feb 19, 2018

15 checks passed

Android Demo App Internal CI build successful
Details
GPU CC Internal CI build successful
Details
GPU Python3 Internal CI build successful
Details
MacOS Contrib Internal CI build successful
Details
MacOS Python2 and CC Internal CI build successful
Details
Ubuntu CC Internal CI build successful
Details
Ubuntu Makefile Internal CI build successful
Details
Ubuntu Python2 Internal CI build successful
Details
Ubuntu Python3 Internal CI build successful
Details
Ubuntu Python3 PIP Internal CI build successful
Details
Ubuntu Sanity Internal CI build successful
Details
Ubuntu contrib Internal CI build successful
Details
Windows CMake Internal CI build successful
Details
XLA Internal CI build successful
Details
cla/google All necessary CLAs are signed
@drpngx

This comment has been minimized.

Member

drpngx commented Feb 19, 2018

Thanks for the ping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment