[ONNX] Improve ONNX Loop export#20445
Conversation
houseroad
left a comment
There was a problem hiding this comment.
I think some folk is adding boolean tensor to pytorch now. Shall we wait a bit for that effort?
Great to know there will be improved native boolean tensor support in pytorch. Is there a timeline for that change? I have some other local changes to better support the export for boolean scalars. I guess that can wait. |
|
Fully ready should be pytorch 1.2. Let's move this change forward then. |
42b14fb to
b851f45
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Nit: Can we check cond_val to determine whether it's necessary to add the cast node?
There was a problem hiding this comment.
Added, note this won't have much use until native bool type support is improved. Currently most cases are passing in either byte or long, which still needs casting on onnx level.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I am resolving the conflicts from my side. So no action is required. |
|
@houseroad merged this pull request in 9a41f44. |
Summary: ~~This is work in progress due to its dependency on multiple pending PRs.~~ - [x] ONNX: Relax constraint on subgraph input/output type & shape check. onnx/onnx#2009 - [x] PyTorch: Add infra to test_pytorch_onnx_caffe2.py to test ScriptModule models. pytorch#20256 This PR should partially resolve pytorch#17531. However, ideally we shouldn't need to put cast(and reshape) node to help the conversion for loop condition. - Added cast node for condition values before entering loop node. The ONNX spec only accepts Bool type, while in PyTorch if the condition value is an output from other node it could potentially have any integral type. - Tidying up the exported ONNX loop subgraph input type & shape. According to ONNX spec, input "M" is exported as 0-d scalar tensor with type int64. input "Cond" is exported as incomplete tensor of type Bool without shape information. This is because through out the iteration, the rank of condition value is dynamic, either 0-d or 1-d, as long as it holds a single value. Pull Request resolved: pytorch#20445 Differential Revision: D15534188 Pulled By: houseroad fbshipit-source-id: d174e778529def05ee666afeee4b8fb27786e320
This is work in progress due to its dependency on multiple pending PRs.This PR should partially resolve #17531. However, ideally we shouldn't need to put cast(and reshape) node to help the conversion for loop condition.