-
Notifications
You must be signed in to change notification settings - Fork 488
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
[Computation Hash] Introduce deterministic hash for user computations #8539
[Computation Hash] Introduce deterministic hash for user computations #8539
Conversation
Test results without the deterministic serialization (instead, relying on the former SerializeAsString()):
So |
5e406d7
to
4372f2e
Compare
LGTM however the tests fail. From what I can tell, both failed tests involve an XlaComputation. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM however the tests fail.
From what I can tell, both failed tests involve an XlaComputation. The test_conditional feeds XlaComputation into an HLO Cond op. The scan feeds an XlaComputation into an HLO While op. So probably the hash is broken for ComputationPtr in a way that causes different graphs to hash into the same value.
Indeed, looking. A first look shows that the resulting hash for both user computations are the same, though the computation IRs differ in the constant parameter:
I'll see what is happening with the hash here. |
50ed7c5
to
a8b5b31
Compare
Thanks for capturing it @tengyifei. I was using the computation object arg after move semantics. |
@bhavya01, it seems that the TPU CI has an issue? Seeing the same failure on other runs: |
a8b5b31
to
72f8c27
Compare
Fixes #8537