-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Updates list of TPU-compatible ops for the TB graph viewer. #4024
Conversation
Tayo, thanks for this. The team is in the midst of a complicated migration to Polymer 3 (#3887), which should finish soon. There will be some delay in getting to this PR. |
@manivaradarajan Sure, no hurry. |
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.
Thanks for updating these!
Googlers: for reference, see b/163430986
'_Retval', | ||
'_TPUCompile', | ||
'_TPUExecute', | ||
'_SendTPUEmbeddingGradients', |
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.
nit: should _Send...
should be alphabetcally sorted before L481?
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.
Done
@@ -364,10 +471,17 @@ module tf.graph.op { | |||
'Switch', | |||
// Ops below are inserted by the compiler. | |||
'_Arg', | |||
'_ArrayToList', | |||
'_FusedBatchNormEx', | |||
'_ListToArray', | |||
'_ParallelConcatUpdate', |
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.
When running SupportedOpsMain
from tensorflow/compiler/tf2xla/tf2xla_supported_ops.cc
, I'm not seeing these ops
_ParallelConcatUpdate
_TPUCompile
_TPUExecute
in the list of results. Is keeping them in the allowlist intentional?
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.
Yes. The idea was to support 'legacy' existing graphs. Although this policy can be changed later.
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.
Stopping accidental merge: this is making changes to files that are about to be deleted.
The migration is done and we can now take in all the changes. Thanks for waiting. Please rebase and update your pull request :) |
ae1d462
to
f8d3b8f
Compare
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.
@psybuzz FYI, in order to incorporate the changes, I rebased against master, fixed conflict, then pushed to tayo's branch.
Will merge when CI is happy.
Apologies for tardiness :). Thanks stephanwlee. Let me know if any other actions are necessary from my side. |
No description provided.