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

Never pass a NoOp value to an op #7460

Merged
merged 3 commits into from
Mar 9, 2023

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Mar 9, 2023

The 'NoOp' op appears in the execution graph (although it shouldn't really), and its output value can sometimes be passed to downstream nodes. Temporarily fix this by filtering it out of the parameters in getParamValue.

TODO(mattSoulanille): Refactor the graph so NoOp and other control dependencies are only used for sorting and don't appear in the execution graph.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

The 'NoOp' op appears in the execution graph (although it shouldn't really), and
its output value can sometimes be passed to downstream nodes. Temporarily fix
this by filtering it out of the parameters in `getParamValue`.

TODO(mattSoulanille): Refactor the graph so NoOp and other control dependencies
are only used for sorting and don't appear in the execution graph.
@mattsoulanille mattsoulanille requested a review from pyu10055 March 9, 2023 02:49
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@pyu10055 pyu10055 requested a review from Linchenn March 9, 2023 18:58
@mattsoulanille mattsoulanille requested a review from chunnienc March 9, 2023 18:58
Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

LGTM

.bazelrc Show resolved Hide resolved
@chunnienc
Copy link
Collaborator

chunnienc commented Mar 9, 2023

It looks like most of the usage of getting tensor list param value treat it as unknown size list, but some of it does extract positional tensors from the list (Code Search, notice the search result only convers function calls in a single line, so the actual use cases may be more.) But I believe this PR won't affect those since param tensors shouldn't come from NoOp node.

@mattsoulanille
Copy link
Member Author

It looks like most of the usage of getting tensor list param value treat it as unknown size list, but some of it does extract positional tensors from the list (Code Search, notice the search result only convers function calls in a single line, so the actual use cases may be more.) But I believe this PR won't affect those since param tensors shouldn't come from NoOp node.

Yeah, and that's why I would like to rewrite this solution later, since it doesn't guarantee the length of the returned list if there's a NoOp node as an input. This kind of error should really be caught in graph analysis instead of model execution. I agree that for correct graphs, we should never encounter this kind of parameter shortening, but I still don't really like it.

@mattsoulanille mattsoulanille merged commit e953e9c into tensorflow:master Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants