[ONNX] add pass for onnx scalar type conversion #24378

BowenBao · 2019-08-14T23:52:50Z

This pass tries to resolve scalar type mismatch issues between input tensors introduced by the implicit type conversions on scalars.

e.g. #23724

spandantiwari · 2019-08-15T00:41:30Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

This mapping maybe useful beyond this pass. We should consider abstracting it out.

spandantiwari · 2019-08-15T00:41:53Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

Maybe this one too.

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

spandantiwari · 2019-08-15T00:47:53Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

This is a method for extending type promotion beyond pairs, to a vector of types. It may be useful beyond this pass. Should we maybe move this as a overload to c10::promoteTypes? @bddppq What do you think?

@smessmer @zdevito do we want to promote this method to c10 level helper function?

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

ajaysg-zz · 2019-08-21T11:03:01Z

@BowenBao and @spandantiwari i cloned the following repo mentioned in this issue https://github.com/BowenBao/pytorch/tree/onnx_implicit_cast. Still i am getting the type mismatch error.

BowenBao · 2019-08-21T18:42:48Z

Hi @ajaysg, on my local machine I could not repro the mismatch error with this branch. Particularly, the exported model now has a cast node after Mul which should resolve this issue.

Please also note that this model won't be able to export, or run in ONNX backend, until the spec & impl for Det and support for torch.unfold is in. We are actively working on these two items.

BowenBao · 2019-08-23T01:36:16Z

@pytorchbot retest this please

ajaysg-zz · 2019-08-23T09:41:08Z

hi @BowenBao i cloned this repo using the following command
git clone https://github.com/BowenBao/pytorch/
then i checkout to the branch using this command
cd pytorch
git checkout -b remotes/origin/onnx_implicit_cast
Then i built the binaries from source
After the installation i converted my model to onnx. I could not find the cast node after mul node. When i tried to use the model in onnxruntime i am getting the same error as before
RuntimeError: [ONNXRuntimeError] : 1 : GENERAL ERROR : Type Error: Type parameter (T) bound to different types (tensor(int64) and tensor(float) in node ().
Thanks for your support in advance

BowenBao · 2019-08-23T20:00:08Z

hi @ajaysg , could you try git checkout -t origin/onnx_implicit_cast? You can use git log to check if your current branch indeed contains patch from this PR.

ajaysg-zz · 2019-08-26T07:24:12Z

Hi @BowenBao i tried the same you suggested. Still i am not able to find the cast node after mul node in the generated onnx model. Also while i am using that model in onnxrt i am getting the Type Parameter issue

facebook-github-bot

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

dzhulgakov · 2019-08-28T03:33:44Z

@pytorchbot retest this please

dzhulgakov · 2019-08-28T03:34:31Z

I'm not the best person to review it probably. @nairbv or @ZolotukhinM - do you happen to have more context on type promotion logic?

facebook-github-bot

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

houseroad

Thanks, this is more fundamental solution for scalar type mismatching solution.

I am not sure whether we have covered all the pytorch ops already or not, but it's good start.

Please address my inline comments.

test/onnx/test_onnx_opset.py

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

torch/onnx/symbolic_opset9.py

houseroad · 2019-09-13T18:53:27Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.h

Not only Analysis, but Propagation?

As below, the pass is not doing Propagation at the moment.

houseroad · 2019-09-13T18:53:46Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.h

Also add some comments about the purpose/motivation of this pass.

houseroad · 2019-09-13T18:56:00Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

It's a bit weird we only have one function call in ScalarTypeAnalysisForONNX, consider merging them?

We were wondering if in the future this is a good place to extend the pass to perform complete shape and type inferencing. Especially to support scripting, which unlike tracing, doesn't record operator outputs with complete tensor type.
There is existing shape_analysis.cpp, but is for aten operators, and it is usually more dynamic than ONNX.

houseroad · 2019-09-13T19:00:48Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

@smessmer @zdevito do we want to promote this method to c10 level helper function?

houseroad · 2019-09-13T19:06:47Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

Give some context why handling like this?

Adding it to the comments.

houseroad · 2019-09-13T19:11:31Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

This may incur some duplicate values, but I guess it should be rare.

ZolotukhinM

The IR pass part looks good!

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

ZolotukhinM · 2019-09-13T19:48:16Z

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp

I wonder if this works correctly if a node has the same value as input several times, e.g

%a = prim::mul(%b, %b)

I'm not sure that inputs() iterator will stay valid after we call replaceInputWith, which would replace all entries of the current input. Could you please double-check that it works as expected? A safe alternative to achieve the same and avoid iterator issues for certain would be to use replaceInput(index, newValue).

That's a very interesting case. I will create a test case for that, after fixing the regression #26328.

resolve & update test issues

BowenBao · 2019-09-17T00:33:02Z

After rebasing, the CI is failing on this test case #26328, which seems to be a regression since it can also repro on master. I'm currently looking at that.

* Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR pytorch#24378 * Fix UTs ghstack-source-id: 4e796b1 Pull Request resolved: pytorch#69548

…JIT pass (#69227)" * Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Differential Revision: [D32994268](https://our.internmc.facebook.com/intern/diff/D32994268) [ghstack-poisoned]

* Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Differential Revision: [D32994268](https://our.internmc.facebook.com/intern/diff/D32994268) [ghstack-poisoned]

…JIT pass (#69227)" * Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Differential Revision: [D32994268](https://our.internmc.facebook.com/intern/diff/D32994268) [ghstack-poisoned]

* Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Differential Revision: [D32994268](https://our.internmc.facebook.com/intern/diff/D32994268) [ghstack-poisoned]

* Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR pytorch#24378 * Fix UTs ghstack-source-id: 4e796b1 Pull Request resolved: pytorch#69548

…JIT pass (#69227)" * Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Differential Revision: [D32994268](https://our.internmc.facebook.com/intern/diff/D32994268) [ghstack-poisoned]

* Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Differential Revision: [D32994268](https://our.internmc.facebook.com/intern/diff/D32994268) [ghstack-poisoned]

Summary: Pull Request resolved: #69548 * Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Test Plan: Imported from OSS Reviewed By: msaroufim Differential Revision: D32994268 Pulled By: malfet fbshipit-source-id: 0deab88b0bb1e396770690af27730accb64fcf63

Summary: Pull Request resolved: #69548 * Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR #24378 * Fix UTs Test Plan: Imported from OSS Reviewed By: msaroufim Differential Revision: D32994268 Pulled By: malfet fbshipit-source-id: 0deab88b0bb1e396770690af27730accb64fcf63 (cherry picked from commit a99322c)

Summary: Pull Request resolved: pytorch/pytorch#69548 * Add Concat to Scalar type analysis pass By using scalar type analysis for Concat, the exported model can do automatic type promotion for Concat nodes, including mixed fp16 and fp32 inputs, for example. Unit tests based on the original PR pytorch/pytorch#24378 * Fix UTs Test Plan: Imported from OSS Reviewed By: msaroufim Differential Revision: D32994268 Pulled By: malfet fbshipit-source-id: 0deab88b0bb1e396770690af27730accb64fcf63 (cherry picked from commit a99322cadf7b79a4548266a9d4d3af094b89bac4)

pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: build Build system issues module: internals Related to internal abstractions in c10 and ATen module: onnx Related to torch.onnx labels Aug 14, 2019

BowenBao requested a review from spandantiwari August 14, 2019 23:54

spandantiwari approved these changes Aug 15, 2019

View reviewed changes

BowenBao force-pushed the onnx_implicit_cast branch from 9665beb to 207cc26 Compare August 19, 2019 17:17

spandantiwari mentioned this pull request Aug 20, 2019

RuntimeError: [ONNXRuntimeError] : 1 : GENERAL ERROR : Load model from waveglow.onnx failed:Type Error: Type parameter (T) bound to different types (tensor(int64) and tensor(float) in node () #23724

Closed

BowenBao force-pushed the onnx_implicit_cast branch 2 times, most recently from ffc18d5 to 0562f98 Compare August 22, 2019 18:10

dzhulgakov requested review from ZolotukhinM and nairbv August 28, 2019 03:32

facebook-github-bot reviewed Aug 28, 2019

View reviewed changes

facebook-github-bot reviewed Sep 13, 2019

View reviewed changes

houseroad requested changes Sep 13, 2019

View reviewed changes

ZolotukhinM reviewed Sep 13, 2019

View reviewed changes

BowenBao added 3 commits September 16, 2019 15:45

Add onnx scalar type conversion pass

2f5cb12

resolve & update test issues

add more comments

ab3a6f5

remove ProfiledTensorType usage

defb318

add more comments

42db938

BowenBao force-pushed the onnx_implicit_cast branch from 0562f98 to 42db938 Compare September 17, 2019 00:50

[ONNX] add pass for onnx scalar type conversion #24378

[ONNX] add pass for onnx scalar type conversion #24378

Uh oh!

Conversation

BowenBao commented Aug 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajaysg-zz commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BowenBao commented Aug 21, 2019

Uh oh!

BowenBao commented Aug 23, 2019

Uh oh!

ajaysg-zz commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BowenBao commented Aug 23, 2019

Uh oh!

ajaysg-zz commented Aug 26, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

dzhulgakov commented Aug 28, 2019

Uh oh!

dzhulgakov commented Aug 28, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BowenBao commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

ajaysg-zz commented Aug 21, 2019 •

edited

Loading

ajaysg-zz commented Aug 23, 2019 •

edited

Loading