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

[Redo of #23090] Clean up binary element-wise assertions #23109

Merged

Conversation

frreiss
Copy link
Contributor

@frreiss frreiss commented Oct 19, 2018

The working branch off of which PR #23090 was based was destroyed in a tragic git accident. This PR is a redo of the same changes.

The reviewers on the original PR were @ymodak and @iganichev. @rohan100jain was also following the original PR.

Original description follows:

TensorFlow 1.5 added two pieces of useful functionality to the assert_equals op. In eager mode, assert_equals now prints a more useful error message that pinpoints which elements of the input tensors differ (see commit 361c55899cb524ca078c65eabdd3d79bfc10c8f9). In graph mode, assert_equals now evaluates the assertion at graph construction time when both inputs can be evaluated statically (see commit cfbeafe11d9b86f8685c1c0f97d285885b5a5f1f).

This PR ports this additional functionality to the other binary element-wise assertion ops assert_none_equal, assert_less, assert_less_equal, assert_greater, and assert_greater_equal.

Before:

In [1]: import numpy as np 
   ...: import tensorflow as tf 
   ...: tf.enable_eager_execution() 
   ...: zeros = np.zeros(1000) 
   ...: mostly_ones = np.full(1000, 1.) 
   ...: mostly_ones[567] = 0. 
   ...: tf.assert_none_equal(zeros, mostly_ones, summarize=3)            
       
---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-23dba1a27a31> in <module>
      5 mostly_ones = np.full(1000, 1.)
      6 mostly_ones[567] = 0.
----> 7 tf.assert_none_equal(zeros, mostly_ones, summarize=3)
[...stack trace continues...]

InvalidArgumentError: Expected 'tf.Tensor(False, shape=(), dtype=bool)' to be true. Summarized data: b''
b'Condition x != y did not hold for every single element:'
b'x (shape=(1000,) dtype=float64) = '
0.0, 0.0, 0.0, ...
b'y (shape=(1000,) dtype=float64) = '
1.0, 1.0, 1.0, ...

After:

In [1]: import numpy as np 
   ...: import tensorflow as tf 
   ...: tf.enable_eager_execution() 
   ...: zeros = np.zeros(1000) 
   ...: mostly_ones = np.full(1000, 1.) 
   ...: mostly_ones[567] = 0. 
   ...: tf.assert_none_equal(zeros, mostly_ones, summarize=3)                
                                       
---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-1c86ae0b9399> in <module>
      5 mostly_ones = np.full(1000, 1.)
      6 mostly_ones[567] = 0.
----> 7 tf.assert_none_equal(zeros, mostly_ones)
[...stack trace continues...]

InvalidArgumentError: Condition x != y did not hold.
Indices of first 1 different values:
[[567]]
Corresponding x values:
[0.]
Corresponding y values:
[0.]
First 3 elements of x:
[0. 0. 0.]
First 3 elements of y:
[1. 1. 1.]

The ops assert_negative, assert_non_negative, assert_positive, and assert_non_positive also get some of the new functionality, as they are based on assert_less and assert_less_equal.

Before:

In [1]: import tensorflow as tf 
   ...: tf.assert_non_negative(-1.)                                             
Out[1]: <tf.Operation 'assert_non_negative/assert_less_equal/Assert/AssertGuard/Merge' type=Merge>

After:

In [1]: import tensorflow as tf 
   ...: tf.assert_non_negative(-1.)                               
                                            
---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-c2ddbad603d6> in <module>
      1 import tensorflow as tf
----> 2 tf.assert_non_negative(-1.)
[...stack trace continues...]

InvalidArgumentError: 
Condition x >= 0 did not hold element-wise:
x (assert_non_negative/x:0) = 
-1.0

I also removed some unnecessary newlines from the error messages and fixed a glitch in the handling of the message parameter when the data parameter is used.
Before:

In [1]: import tensorflow as tf 
   ...: tf.assert_equal(1., 0., data=[3.], message="My error message")          

---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-ef2b26ad5e73> in <module>
      1 import tensorflow as tf
----> 2 tf.assert_equal(1., 0., data=[3.], message="My error message")
                                    [snip!]
InvalidArgumentError: 3.0

After:

In [1]: import tensorflow as tf 
   ...: tf.assert_equal(1., 0., data=[3.], message="My error message")                  
                      
---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-ef2b26ad5e73> in <module>
      1 import tensorflow as tf
----> 2 tf.assert_equal(1., 0., data=[3.], message="My error message")
[...stack trace continues...]

InvalidArgumentError: My error message
3.0

In the process, I replaced a bunch of near-duplicate code and documentation across the assert_* functions with a single function (_binary_assert() in check_ops.py) and common blocks of documentation (_binary_assert_doc() and _unary_assert_doc() in check_ops.py). check_ops.py is now about 125 lines shorter.

I added some new regression tests to cover static assertion checks in graph mode and modified some existing tests to account for the new functionality.

I built a local copy of the documentation for the tf.debugging package and reviewed all the resulting Markdown files.

@frreiss
Copy link
Contributor Author

frreiss commented Oct 19, 2018

Review comments from previous PR:

1
Reviewer: iganichev
Location: tensorflow/python/ops/check_ops.py line 202
Comment: 'eq' should be 'test' here.
Status: Resolved by current changes

2
Reviewer: iganichev
Location: tensorflow/python/ops/check_ops.py line 181 (now line 312)
Comment: nit: Since you are returning above, no need for the "else" and extra indentation.
Status: Not yet resolved; will fix shortly

3
Reviewer: iganichev
Location: tensorflow/python/ops/check_ops.py line 228
Comment: Define outside the function as something like _pretty_print to avoid redefinition overhead on every call
Status: Resolved by current changes

4
Reviewer: iganichev
Location: tensorflow/python/ops/check_ops.py line 217
Comment: does not seem necessary
Status: Resolved by current changes

5
Reviewer: iganichev
Location: tensorflow/python/ops/check_ops_test.py line 516
Comment: assert_none_equal -> assert_less
Status: Not yet resolved; will fix shortly

6
Reviewer: iganichev
Location: tensorflow/python/ops/check_ops.py line 1363
Comment: -1 is non_positive. If this test passes, assert_non_positive has a bug.
Status: Resolved by current changes

@ymodak ymodak self-assigned this Oct 19, 2018
@ymodak ymodak requested review from rohan100jain and iganichev and removed request for rohan100jain October 19, 2018 20:22
@ymodak ymodak added the awaiting review Pull request awaiting review label Oct 19, 2018
tensorflow/python/ops/check_ops.py Outdated Show resolved Hide resolved
tensorflow/python/ops/check_ops.py Outdated Show resolved Hide resolved
@frreiss
Copy link
Contributor Author

frreiss commented Oct 19, 2018

Pushed some additional changes to address open review comments from @iganichev.

Thanks for the review, and sorry for creating additional work for you with the commit structure of the previous PR.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 21, 2018
iganichev
iganichev previously approved these changes Oct 22, 2018
@ymodak ymodak added kokoro:force-run Tests on submitted change awaiting review Pull request awaiting review labels Oct 23, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 23, 2018
@ymodak ymodak added awaiting testing (then merge) kokoro:force-run Tests on submitted change and removed awaiting review Pull request awaiting review labels Oct 23, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 24, 2018
@ymodak
Copy link
Contributor

ymodak commented Oct 25, 2018

@frreiss Can you please make changes to address test failures to proceed with the PR? Please let me know if you need any information. Thanks!

@ymodak ymodak added the awaiting review Pull request awaiting review label Oct 25, 2018
@iganichev
Copy link
Contributor

@frreiss I looked at a few failures. They either expect a wrong string message in the error of different exception type (because we did not use static checking in all the asserts). They should be fairly straightforward to fix.

@ymodak ymodak added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Oct 25, 2018
@frreiss
Copy link
Contributor Author

frreiss commented Oct 26, 2018

Looking into the test failures now.

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Oct 27, 2018
@rthadur rthadur added ready to pull PR ready for merge process pending merge internally and removed ready to pull PR ready for merge process labels Jun 24, 2019
@rthadur rthadur added ready to pull PR ready for merge process kokoro:force-run Tests on submitted change and removed ready to pull PR ready for merge process labels Jul 3, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 3, 2019
@frreiss
Copy link
Contributor Author

frreiss commented Jul 22, 2019

Can someone look at the log from that failing copybara job and let me know what went wrong? I don't have access to that CI server.

@rthadur rthadur added ready to pull PR ready for merge process and removed pending merge internally ready to pull PR ready for merge process labels Jul 22, 2019
@alextp
Copy link
Contributor

alextp commented Jul 23, 2019

tensorflow/contrib/neural_link:utils_test is failing, with the following message

======================================================================
ERROR: testApplyFeatureMaskWithInvalidMaskNegative (__main__.DecayOverTimeTest)
testApplyFeatureMaskWithInvalidMaskNegative (__main__.DecayOverTimeTest)
Test the apply_feature_mask function with mask value < 0.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/py/absl/third_party/unittest3_backport/case.py", line 37, in testPartExecutor
    yield
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/py/absl/third_party/unittest3_backport/case.py", line 162, in run
    testMethod()
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/contrib/neural_link/utils_test.py", line 261, in testApplyFeatureMaskWithInvalidMaskNegative
    constant(features), constant(mask))
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/contrib/neural_link/utils.py", line 226, in apply_feature_mask
    check_ops.assert_non_negative(feature_mask),
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/python/ops/check_ops.py", line 560, in assert_non_negative
    return assert_less_equal(zero, x, data=data, summarize=summarize)
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/python/ops/check_ops.py", line 922, in assert_less_equal
    name)
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/python/ops/check_ops.py", line 371, in _binary_assert
    _assert_static(condition_static, data)
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/python/ops/check_ops.py", line 86, in _assert_static
    message='\n'.join(data_static))
InvalidArgumentError: 
Condition x >= 0 did not hold element-wise:
x (Const_1:0) = 
[-1.  1.]

======================================================================
ERROR: testApplyFeatureMaskWithInvalidMaskTooLarge (__main__.DecayOverTimeTest)
testApplyFeatureMaskWithInvalidMaskTooLarge (__main__.DecayOverTimeTest)
Test the apply_feature_mask function with mask value > 1.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/py/absl/third_party/unittest3_backport/case.py", line 37, in testPartExecutor
    yield
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/py/absl/third_party/unittest3_backport/case.py", line 162, in run
    testMethod()
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/contrib/neural_link/utils_test.py", line 271, in testApplyFeatureMaskWithInvalidMaskTooLarge
    constant(features), constant(mask))
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/contrib/neural_link/utils.py", line 228, in apply_feature_mask
    feature_mask, constant_op.constant(1, dtype=feature_mask.dtype))
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/python/ops/check_ops.py", line 922, in assert_less_equal
    name)
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/python/ops/check_ops.py", line 371, in _binary_assert
    _assert_static(condition_static, data)
  File "/build/work/59fe95cabf7ec270c279aa86384651aff604/google3/runfiles/google3/third_party/tensorflow/python/ops/check_ops.py", line 86, in _assert_static
    message='\n'.join(data_static))
InvalidArgumentError: Condition x <= y did not hold element-wise:
x (Const_1:0) = 
[ 1.  2.]
y (Const_2:0) = 
1.0

----------------------------------------------------------------------

@alextp
Copy link
Contributor

alextp commented Jul 23, 2019

I'm fine with disabling this test / reverting assertion changes which affect it

@rthadur rthadur added the stat:awaiting response Status - Awaiting response from author label Jul 23, 2019
@frreiss
Copy link
Contributor Author

frreiss commented Jul 23, 2019

I don't have the source code that's being exercised there. But from the stack trace, it looks like the test case testApplyFeatureMaskWithInvalidMaskNegative in contrib/neural_link/utils_test.py, deliberately passes an invalid value to the apply_feature_mask() function. The test case expects this invalid value to generate an exception later on when running the graph, but after the changes in this PR, the invalid value generates an exception immediately. The same thing happens with testApplyFeatureMaskWithInvalidMaskTooLarge in the same source file.

If this diagnosis is correct, you can fix the failing tests by wrapping the call to apply_feature_mask in a with self.assertRaisesRegexp(errors.InvalidArgumentError) context manager and removing the part of the test case that calls Session.run(). I made a similar change at line 44 of tensorflow/contrib/distributions/python/kernel_tests/deterministic_test.py in this PR.

Or you can just disable the test cases testApplyFeatureMaskWithInvalidMaskNegative and testApplyFeatureMaskWithInvalidMaskTooLarge in tensorflow/contrib/neural_link/utils_test.py

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Jul 24, 2019
@rthadur
Copy link
Contributor

rthadur commented Jul 24, 2019

@frreiss here is the internal error

Traceback (most recent call last): File "/py/absl/third_party/unittest3_backport/case.py", line 37, in testPartExecutor yield File "py/absl/third_party/unittest3_backport/case.py", line 162, in run testMethod() File "tensorflow/contrib/neural_link/utils_test.py", line 261, in testApplyFeatureMaskWithInvalidMaskNegative constant(features), constant(mask)) File "/tensorflow/contrib/neural_link/utils.py", line 226, in apply_feature_mask check_ops.assert_non_negative(feature_mask), File "tensorflow/python/ops/check_ops.py", line 560, in assert_non_negative return assert_less_equal(zero, x, data=data, summarize=summarize) File "tensorflow/python/ops/check_ops.py", line 922, in assert_less_equal name) File "tensorflow/python/ops/check_ops.py", line 371, in _binary_assert _assert_static(condition_static, data) File "ensorflow/python/ops/check_ops.py", line 86, in _assert_static message='\n'.join(data_static)) InvalidArgumentError: Condition x >= 0 did not hold element-wise: x (Const_1:0) = [-1. 1.]

@frreiss
Copy link
Contributor Author

frreiss commented Jul 24, 2019

@frreiss here is the internal error

[. . .]

Thanks for that info, @rthadur. That traceback seems to be the same one that @alextp posted above. Looking at the stack trace again, I'm pretty confident in my assessment from yesterday: This PR adds a static check to assert_less_equal, and the test case triggers that static check at graph construction time instead of triggering the old dynamic check at graph execution time.

If you wrap the statement at line 260 of tensorflow/contrib/neural_link/utils_test.py in a with self.assertRaisesRegexp(errors.InvalidArgumentError, "Condition x >= 0"): context manager and delete the parts of testApplyFeatureMaskWithInvalidMaskNegative() that come after line 261, the test should pass.

@iganichev
Copy link
Contributor

@frreiss Sorry for delay. I am fixing the internal errors now.

mihaimaruseac pushed a commit to tensorflow/estimator that referenced this pull request Aug 1, 2019
…ion time

This is purely refactoring that is compatible with current asserts firing
during session.run and newer asserts that sometime fire at op creation time.
It is needed to submit tensorflow/tensorflow#23109.

PiperOrigin-RevId: 261207674
@iganichev
Copy link
Contributor

iganichev commented Aug 2, 2019

@frreiss. I submitted the change but it broke quite a few more tests in various projects I did not anticipate and they are not as straightforward as before - when we are trying to get a constant value from a tensor "x_static = tensor_util.constant_value(x)", the new tensor x_static becomes unfeedable - users cannot change it in feed_dict. Some tests try to change the value after going through some assert op.

I am rolling it back and will try to resubmit after fixing all the issues.

@tensorflow-copybara tensorflow-copybara merged commit 5b73371 into tensorflow:master Aug 2, 2019
tensorflow-copybara pushed a commit that referenced this pull request Aug 2, 2019
tensorflow-copybara pushed a commit that referenced this pull request Aug 2, 2019
@frreiss
Copy link
Contributor Author

frreiss commented Aug 6, 2019

Thanks, @iganichev, I appreciate the help in shepherding this change.

tensorflow-copybara pushed a commit that referenced this pull request Aug 16, 2019
Imported from GitHub PR #23109

**The working branch off of which PR #23090 was based was destroyed in a tragic git accident. This PR is a redo of the same changes.**

**The reviewers on the original PR were @ymodak and @iganichev. @rohan100jain was also following the original PR.**

**Original description follows:**

TensorFlow 1.5 added two pieces of useful functionality to the `assert_equals` op. In eager mode, `assert_equals` now prints a more useful error message that pinpoints which elements of the input tensors differ (see commit 361c558). In graph mode, `assert_equals` now evaluates the assertion at graph construction time when both inputs can be evaluated statically (see commit cfbeafe).

This PR ports this additional functionality to the other binary element-wise assertion ops `assert_none_equal`, `assert_less`, `assert_less_equal`, `assert_greater`, and `assert_greater_equal`.

**Before:**
```
In [1]: import numpy as np
   ...: import tensorflow as tf
   ...: tf.enable_eager_execution()
   ...: zeros = np.zeros(1000)
   ...: mostly_ones = np.full(1000, 1.)
   ...: mostly_ones[567] = 0.
   ...: tf.assert_none_equal(zeros, mostly_ones, summarize=3)

---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-23dba1a27a31> in <module>
      5 mostly_ones = np.full(1000, 1.)
      6 mostly_ones[567] = 0.
----> 7 tf.assert_none_equal(zeros, mostly_ones, summarize=3)
[...stack trace continues...]

InvalidArgumentError: Expected 'tf.Tensor(False, shape=(), dtype=bool)' to be true. Summarized data: b''
b'Condition x != y did not hold for every single element:'
b'x (shape=(1000,) dtype=float64) = '
0.0, 0.0, 0.0, ...
b'y (shape=(1000,) dtype=float64) = '
1.0, 1.0, 1.0, ...
```
**After:**
```
In [1]: import numpy as np
   ...: import tensorflow as tf
   ...: tf.enable_eager_execution()
   ...: zeros = np.zeros(1000)
   ...: mostly_ones = np.full(1000, 1.)
   ...: mostly_ones[567] = 0.
   ...: tf.assert_none_equal(zeros, mostly_ones, summarize=3)

---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-1c86ae0b9399> in <module>
      5 mostly_ones = np.full(1000, 1.)
      6 mostly_ones[567] = 0.
----> 7 tf.assert_none_equal(zeros, mostly_ones)
[...stack trace continues...]

InvalidArgumentError: Condition x != y did not hold.
Indices of first 1 different values:
[[567]]
Corresponding x values:
[0.]
Corresponding y values:
[0.]
First 3 elements of x:
[0. 0. 0.]
First 3 elements of y:
[1. 1. 1.]
```

The ops `assert_negative`, `assert_non_negative`, `assert_positive`, and `assert_non_positive` also get some of the new functionality, as they are based on `assert_less` and `assert_less_equal`.

**Before:**
```
In [1]: import tensorflow as tf
   ...: tf.assert_non_negative(-1.)
Out[1]: <tf.Operation 'assert_non_negative/assert_less_equal/Assert/AssertGuard/Merge' type=Merge>
```
**After:**
```
In [1]: import tensorflow as tf
   ...: tf.assert_non_negative(-1.)

---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-c2ddbad603d6> in <module>
      1 import tensorflow as tf
----> 2 tf.assert_non_negative(-1.)
[...stack trace continues...]

InvalidArgumentError:
Condition x >= 0 did not hold element-wise:
x (assert_non_negative/x:0) =
-1.0
```

I also removed some unnecessary newlines from the error messages and fixed a glitch in the handling of the `message` parameter when the `data` parameter is used.
**Before:**
```
In [1]: import tensorflow as tf
   ...: tf.assert_equal(1., 0., data=[3.], message="My error message")

---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-ef2b26ad5e73> in <module>
      1 import tensorflow as tf
----> 2 tf.assert_equal(1., 0., data=[3.], message="My error message")
                                    [snip!]
InvalidArgumentError: 3.0
```
**After:**
```
In [1]: import tensorflow as tf
   ...: tf.assert_equal(1., 0., data=[3.], message="My error message")

---------------------------------------------------------------------------
InvalidArgumentError                      Traceback (most recent call last)
<ipython-input-1-ef2b26ad5e73> in <module>
      1 import tensorflow as tf
----> 2 tf.assert_equal(1., 0., data=[3.], message="My error message")
[...stack trace continues...]

InvalidArgumentError: My error message
3.0
```

In the process, I replaced a bunch of near-duplicate code and documentation across the `assert_*` functions with a single function (`_binary_assert()` in `check_ops.py`) and common blocks of documentation (`_binary_assert_doc()` and `_unary_assert_doc()` in `check_ops.py`). `check_ops.py` is now about 125 lines shorter.

I added some new regression tests to cover static assertion checks in graph mode and modified some existing tests to account for the new functionality.

I built a local copy of the documentation for the `tf.debugging` package and reviewed all the resulting Markdown files.

PiperOrigin-RevId: 263692234
mihaimaruseac pushed a commit to tensorflow/estimator that referenced this pull request Sep 18, 2019
…ion time

This is purely refactoring that is compatible with current asserts firing
during session.run and newer asserts that sometime fire at op creation time.
It is needed to submit tensorflow/tensorflow#23109.

PiperOrigin-RevId: 261207674
@frreiss frreiss deleted the issue-assert-refactor-2 branch December 2, 2019 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:XL CL Change Size:Extra Large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants