Update the onnx Gemm op to FC/FCTransposed logic in caffe2 onnx backend by JerryShih · Pull Request #10108 · pytorch/pytorch

JerryShih · 2018-08-01T03:58:51Z

The broadcast is used by default when the opset version is greater then 6.
#9874

JerryShih · 2018-08-01T04:03:04Z

@houseroad @yinghai
How about this update?

JerryShih · 2018-08-01T05:30:45Z

@houseroad
There are some shape mismatch bugs in my patch. Please skip this reviewing request.
I will update for these bugs soon.

JerryShih · 2018-08-02T12:25:52Z

@houseroad
Could you please review these patch?

caffe2/python/onnx/tests/c2_ref_test.py

houseroad

Overall looks pretty good.

Nit: use int instead of bool when calling make_node, since transA, transB, and broadcast are all int.

JerryShih · 2018-08-05T09:57:47Z

@houseroad
Thank you. All boolean values are converted to int.
Wait for the CI test again.

JerryShih · 2018-08-05T11:32:03Z

@houseroad
CI checks have passed

facebook-github-bot

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

caffe2/python/onnx/tests/c2_ref_test.py

bddppq

I think what this PR is trying to improve is to use FCTransposed when trans_b is False, this is a good idea. Could you update the pull request's title & summary accordingly?

caffe2/onnx/backend.cc

@houseroad

@houseroad

houseroad · 2018-08-06T04:39:55Z

A potential follow up change (after getting this merged):

Right now, FC only accpet 1d bias, however, we can improve FC's implementation to accept 2d bias.

FC's implementation:
https://github.com/pytorch/pytorch/blob/master/caffe2/operators/fully_connected_op.h#L69

Same to FCTranspose.

When FC accepts 2d bias, we can always turn a Gemm into Scale + FC(Transpose)

JerryShih · 2018-08-06T16:42:54Z

@houseroad @bddppq
I update some comments in review page.

JerryShih · 2018-08-10T00:47:31Z

@houseroad @bddppq
What do you think for my comments? Did I miss something?

Update the _known_opset_version value as the opset_version in 'caffe2/python/onnx/backend.py'.

1. support broadcast by default if the opset version is greater than 6. 2. use FC/FCTransposed if we have broadcast and the input_c's dim matching the FC/FCTransposed's requirement.

If we have broadcast and the input_c's dim is not 1, we still can't use FC for this case. So, remove this wrong test.

1. The opset7 uses broadcast by default. Set the explicit opset version to test the different behavior. 2. Add the new FCTransposed op test case.

JerryShih · 2018-08-14T21:55:36Z

@houseroad @bddppq
Request review again.

I update the condition for converting to FC.

If c is "1d" tensor and c is not a scalar, use FC.
If c is a scalar and b's shape is not available, skip FC.
If c is a scalar and a*b's last dim is 1, use FC.
If c is a scalar and a*b's last dim is not 1, skip FC.

In most case, we will use FC.

JerryShih · 2018-08-18T04:05:11Z

@houseroad @bddppq
Any feedback?

"pr/caffe2-py2-cuda9.0-cudnn7-windows-build — Build failed"
This test always failed, but that's not related to these patches.

houseroad · 2018-08-18T04:11:32Z

I have restarted the build, don’t worry. I will review today later.

facebook-github-bot

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

JerryShih · 2018-08-20T20:13:41Z

@houseroad
It already has taken 3 days waiting for merging pr. Is it any test failed?

houseroad · 2018-08-20T20:16:08Z

I am running some internal test cases. Will update it soon.

houseroad · 2018-08-20T23:19:57Z

@JerryShih are you interested in improving FC and FCTranspose in Caffe2?

JerryShih · 2018-08-20T23:35:02Z

yes, I will do that.

houseroad · 2018-08-20T23:36:15Z

Awesome, if you have any question, feel free to ping on gitter.

* upstream/master: (147 commits) Support Loading to GPU (pytorch#10710) More changes for hidden visibility (pytorch#10692) Add arguments __repr__ in Distribution base class Add util function from core type to dtype (pytorch#10716) Make ONNX_ATEN_FALLBACK as internal default option Set the BUILD_ENVIRONMENT variable before installing sccache. (pytorch#10640) Avoid shadowing i, j vars in GeneralProposals test (pytorch#10721) Move THNN Reduction to ATen/core. (pytorch#10703) Completely remove build_aten and use_aten (pytorch#10469) Make empty list literals construct empty Tensor[] (pytorch#10705) Soumith's last few patches to v0.4.1 Fix issues link in Caffe2 readme (pytorch#10711) Update the onnx Gemm op to FC/FCTransposed logic in caffe2 onnx backend (pytorch#10108) Adding ATEN_NO_TEST option to root level cmake for propogation to aten Allow method-style casts on tensors (pytorch#10641) Fix pytorch#10698 build failure (pytorch#10704) Add support for Log() Add a bisect percentile operator (pytorch#10563) Fix EnsureCPUOutputOp (pytorch#10651) Nomnigraph - rename some APIs that invole Subtree to Subgraph (pytorch#10551) ...

…nd (pytorch#10108) Summary: The broadcast is used by default when the opset version is greater then 6. Pull Request resolved: pytorch#10108 Reviewed By: bddppq Differential Revision: D9176934 Pulled By: houseroad fbshipit-source-id: b737bd87b0ddc241c657d35856d1273c9950eeba

yinghai requested a review from houseroad August 1, 2018 04:30

ezyang changed the title ~~Use broadcast by default for Gemm op when the opset version is greater then 6.~~ [WIP] Use broadcast by default for Gemm op when the opset version is greater then 6. Aug 1, 2018

JerryShih force-pushed the use-FC-for-Gemm branch 2 times, most recently from a09df51 to 26520a7 Compare August 1, 2018 21:31

JerryShih changed the title ~~[WIP] Use broadcast by default for Gemm op when the opset version is greater then 6.~~ [WIP] Update the onnx Gemm op to FC logic in caffe2 onnx backend Aug 1, 2018

JerryShih force-pushed the use-FC-for-Gemm branch from 26520a7 to c94eb43 Compare August 2, 2018 08:12

JerryShih changed the title ~~[WIP] Update the onnx Gemm op to FC logic in caffe2 onnx backend~~ Update the onnx Gemm op to FC logic in caffe2 onnx backend Aug 2, 2018

houseroad reviewed Aug 4, 2018

View reviewed changes

caffe2/python/onnx/tests/c2_ref_test.py Outdated

This comment was marked as off-topic.

Sign in to view

houseroad reviewed Aug 4, 2018

View reviewed changes

caffe2/python/onnx/tests/c2_ref_test.py Outdated

This comment was marked as off-topic.

Sign in to view

houseroad reviewed Aug 4, 2018

View reviewed changes

caffe2/python/onnx/tests/c2_ref_test.py Outdated

This comment was marked as off-topic.

Sign in to view

houseroad approved these changes Aug 4, 2018

View reviewed changes

JerryShih force-pushed the use-FC-for-Gemm branch from c94eb43 to 05c571f Compare August 5, 2018 09:55

facebook-github-bot reviewed Aug 5, 2018

View reviewed changes

houseroad reviewed Aug 6, 2018

View reviewed changes

caffe2/python/onnx/tests/c2_ref_test.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

bddppq previously requested changes Aug 6, 2018

View reviewed changes

JerryShih changed the title ~~Update the onnx Gemm op to FC logic in caffe2 onnx backend~~ Update the onnx Gemm op to FC/FCTransposed logic in caffe2 onnx backend Aug 6, 2018

JerryShih force-pushed the use-FC-for-Gemm branch from 05c571f to 44ef2af Compare August 14, 2018 09:30

fmassa added the caffe2 label Aug 14, 2018

JerryShih added 3 commits August 15, 2018 05:44

Update the known_opset_version as the python onnx backend

dee1bfb

Update the _known_opset_version value as the opset_version in 'caffe2/python/onnx/backend.py'.

Update the onnx Gemm op to FC/FCTransposed logic in caffe2 onnx backend.

00531c4

1. support broadcast by default if the opset version is greater than 6. 2. use FC/FCTransposed if we have broadcast and the input_c's dim matching the FC/FCTransposed's requirement.

Remove the broadcast test case in Gemm test.

73fab1b

If we have broadcast and the input_c's dim is not 1, we still can't use FC for this case. So, remove this wrong test.

Update Gemm test case.

912e3b5

1. The opset7 uses broadcast by default. Set the explicit opset version to test the different behavior. 2. Add the new FCTransposed op test case.

JerryShih force-pushed the use-FC-for-Gemm branch from 44ef2af to 912e3b5 Compare August 14, 2018 21:46

facebook-github-bot reviewed Aug 18, 2018

View reviewed changes

facebook-github-bot closed this in 826550a Aug 20, 2018

ezyang added open source merged labels Jun 24, 2019

Conversation

JerryShih commented Aug 1, 2018

Uh oh!

JerryShih commented Aug 1, 2018

Uh oh!

JerryShih commented Aug 1, 2018

Uh oh!

JerryShih commented Aug 2, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

JerryShih commented Aug 5, 2018

Uh oh!

JerryShih commented Aug 5, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

bddppq left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

houseroad commented Aug 6, 2018

Uh oh!

JerryShih commented Aug 6, 2018

Uh oh!

JerryShih commented Aug 10, 2018

Uh oh!

JerryShih commented Aug 14, 2018

Uh oh!

JerryShih commented Aug 18, 2018

Uh oh!

houseroad commented Aug 18, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

JerryShih commented Aug 20, 2018

Uh oh!

houseroad commented Aug 20, 2018

Uh oh!

houseroad commented Aug 20, 2018

Uh oh!

JerryShih commented Aug 20, 2018