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

[Relay][TensorFlow] Add support for SquaredDifference #3930

Merged
merged 5 commits into from
Sep 15, 2019

Conversation

soiferj
Copy link
Contributor

@soiferj soiferj commented Sep 10, 2019

Add support for SquaredDifference operator. Also include a minor bug fix for BatchMatMul.

@srkreddy1238 @alexeyr would you be able to take a look?

@alexeyr
Copy link
Contributor

alexeyr commented Sep 12, 2019

I don't have a problem with BatchMatMul change, but what is the bug it fixes? And should other attr['_output_shapes'] uses be changed too?

@soiferj
Copy link
Contributor Author

soiferj commented Sep 12, 2019

I was testing out a BERT model which was exported with (add_shapes=True), but then ran the GraphTransform FoldConstants. For whatever reason, this seemed to remove shape information from the graph. In my testing, this was the only operator that threw this issue.

I don't think we need to do this for each operator quite yet. This fix is just a more future-proof way of coding this logic.

@yongwww
Copy link
Member

yongwww commented Sep 12, 2019

I was testing out a BERT model which was exported with (add_shapes=True), but then ran the GraphTransform FoldConstants. For whatever reason, this seemed to remove shape information from the graph. In my testing, this was the only operator that threw this issue.

I don't think we need to do this for each operator quite yet. This fix is just a more future-proof way of coding this logic.

What happened if w/o (add_shapes=True)?

@soiferj
Copy link
Contributor Author

soiferj commented Sep 12, 2019

Sorry, can you explain what you mean?

@yongwww
Copy link
Member

yongwww commented Sep 12, 2019

@soiferj the tf converter works even without specifying add_shapes=True actually. So just wondering if there are any issues for the exported bert without explicitly specifying add_shapes=True.

Copy link
Member

@yongwww yongwww left a comment

Choose a reason for hiding this comment

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

LGTM

@soiferj
Copy link
Contributor Author

soiferj commented Sep 12, 2019

I see. I am not sure, I didn't try that out. However, we may want to think about not relaying on _output_shape in the future.

@yongwww
Copy link
Member

yongwww commented Sep 13, 2019

@soiferj if your bert model is working, it will be nice to add an end to end test case with that model.

@soiferj
Copy link
Contributor Author

soiferj commented Sep 13, 2019

Agreed. I will work on adding that in another PR once I’ve verified everything looks ok after this change.

@lixiaoquan
Copy link
Contributor

I see. I am not sure, I didn't try that out. However, we may want to think about not relaying on _output_shape in the future.

It works, but it is very slow. Without shape information got from TF, Frontend will have to infer type for each new added node. For a model containing about 8000 nodes, it will take more than 1.5 hours to import a graph.

@tqchen
Copy link
Member

tqchen commented Sep 13, 2019

@zhiics please help to manage this PR

@zhiics
Copy link
Member

zhiics commented Sep 13, 2019

The change now looks good to me.

@alexeyr @yongwww @lixiaoquan please take another look and https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

@zhiics zhiics merged commit 0482623 into apache:master Sep 15, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
* Add support for SquaredDifference and StopGradient; minor fix in BatchMatMul

* Remove stopgradient change

* Resolve PR comment

* Dummy change to retrigger CI

* dummy change to retrigger CI
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
* Add support for SquaredDifference and StopGradient; minor fix in BatchMatMul

* Remove stopgradient change

* Resolve PR comment

* Dummy change to retrigger CI

* dummy change to retrigger CI
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 16, 2019
* Add support for SquaredDifference and StopGradient; minor fix in BatchMatMul

* Remove stopgradient change

* Resolve PR comment

* Dummy change to retrigger CI

* dummy change to retrigger CI
@soiferj soiferj deleted the soiferj/tfupdates branch September 16, 2019 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants