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

Default EQUALITY_OPERATORS in Autograph #55317

Closed
wants to merge 18 commits into from

Conversation

bhack
Copy link
Contributor

@bhack bhack commented Mar 21, 2022

fix #55278

/cc @mdanatg Let me know where do you want to move the test so that I write it like an integration test as where is now it will go to fail for missing variable conversion (ag__.ld).

@bhack bhack requested a review from mdanatg as a code owner March 21, 2022 22:07
@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Mar 21, 2022
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Mar 21, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 21, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 21, 2022
@mdanatg mdanatg changed the title Default EQUALITY_OPERATOIRS in Autograph Default EQUALITY_OPERATORS in Autograph Mar 21, 2022
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Mar 22, 2022
@gbaned gbaned added the prtype:bugfix PR to fix a bug label Mar 22, 2022
@bhack
Copy link
Contributor Author

bhack commented Mar 22, 2022

I've extended an high level integration test.
Let me know if we want to remove or we could modify the lowlevel one to pass.

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 22, 2022
@bhack bhack requested a review from mdanatg March 22, 2022 10:56
@bhack bhack requested a review from mdanatg March 22, 2022 16:44
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 27, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 27, 2022
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 29, 2022
@bhack
Copy link
Contributor Author

bhack commented Mar 29, 2022

Copybara again

Copy link

@mdanatg mdanatg left a comment

Choose a reason for hiding this comment

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

BTW, I finally exported the integration tests - they're now under autograph/tests.

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Mar 29, 2022
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 29, 2022
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 29, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 29, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 29, 2022
from tensorflow.python.framework import test_util
from tensorflow.python.platform import test


class LogicalExpressionTest(converter_testing.TestCase):

def test_equals(self):


def f(a, b):
return a == b
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is see here is that here in the test this a python bool.
Where in a standard gist like this is a bool Tensor

def test(a, b):
  print(a == b)
  return a == b

a = tf.constant([1])
b = 2
test(a, b)
tf.Tensor([ True], shape=(1,), dtype=bool)

Copy link

Choose a reason for hiding this comment

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

That's in a standalone shell, right? The unit test might be running in different conditions, likely in graph mode. I fear the best way to debug the error is to run the test, with both --test_env=TF2_BEHAVIOR=1 and =0...

Copy link
Contributor Author

@bhack bhack Mar 30, 2022

Choose a reason for hiding this comment

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

That's in a standalone shell, right

it was on colab of course in graph mode was Tensor("Equal:0", shape=(1,), dtype=bool) that is the similar when in the test I use another operator like e.g. >=

--test_env=TF2_BEHAVIOR=1 and =0

It always print a Python bool in the test

Copy link
Contributor Author

@bhack bhack Mar 30, 2022

Choose a reason for hiding this comment

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

I have committed an alternative test and it is passing. You can compare the results in the log.

If there is something not working in ops.enable_tensor_equality() I suppose that it need to be covered with an additional use case in its test file:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/eager/core_test.py#L92

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 30, 2022
@gbaned gbaned requested a review from mdanatg March 31, 2022 14:21
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Mar 31, 2022
copybara-service bot pushed a commit that referenced this pull request Mar 31, 2022
@bhack
Copy link
Contributor Author

bhack commented Apr 4, 2022

@mdanatg I suppose this was merged with b462db3.
But I have totally lost my attribution on git:

git show -s --format="%ae" b462db3

gardener@tensorflow.org

@bhack
Copy link
Contributor Author

bhack commented Apr 4, 2022

P.s. Ok I found it on git show -s --format="%ce %ae" e2cc982. I suppose we could close this.

@mdanatg
Copy link

mdanatg commented Apr 4, 2022

Yes, it's strange that GH didn't automatically marked it as merged. Probably due to the internal revisions.

@mdanatg mdanatg closed this Apr 4, 2022
PR Queue automation moved this from Approved by Reviewer to Closed/Rejected Apr 4, 2022
@google-ml-butler google-ml-butler bot removed the awaiting review Pull request awaiting review label Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prtype:bugfix PR to fix a bug size:S CL Change Size: Small
Projects
PR Queue
  
Closed/Rejected
Development

Successfully merging this pull request may close these issues.

tf.function condition with tensor
5 participants