PyTorch export to ONNX Opset 7 and 8 #21765
Conversation
add a comment about _registry
pytorchbot
added
oncall: jit
|
@houseroad appreciate if you have time to review this PR. Thanks. |
zhangguanheng66
added
the
triaged
facebook-github-bot
left a comment
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.
houseroad
left a comment
houseroad
left a comment
There was a problem hiding this comment.
Overall, it's useful to support opset 7 and 8. However, we should not let opset 7 and 8 reply on opset 9. Instead, it should be the opposite way.
| case at::ScalarType::Int: | ||
| case at::ScalarType::Short: | ||
| case at::ScalarType::Bool: | ||
| to_type = 6; // Int32 |
There was a problem hiding this comment.
Nit: use Enum instead of using int directly.
| break; | ||
|
|
||
| case at::ScalarType::Long: | ||
| to_type = 7; // Int64 |
| using namespace ::c10::onnx; | ||
| } | ||
|
|
||
| void CastConstant(Block* block) { |
There was a problem hiding this comment.
What's the motivation of adding this CastConstant pass?
| // see .cpp for docs | ||
| TORCH_API void CastConstant(const std::shared_ptr<Graph>& graph); | ||
| } // namespace jit | ||
| } // namespace torch No newline at end of file |
| from torch.onnx.symbolic_helper import _black_list_in_opset | ||
| from torch.onnx.symbolic_opset9 import _adaptive_pool, _pair, _single, _triple, max_pool1d, max_pool2d, max_pool3d | ||
|
|
||
| black_listed_operators = ["scan", "expand"] |
There was a problem hiding this comment.
the right way to add support for opset 7 and opset 8 is putting all the operators in the black_list_operators... Let's don't import anything from opset 9, since it's very tricky to get it correct this way.
|
|
||
| # if a opset version is less than 9 check if model has a constant int node | ||
| # and if so add a cast to it | ||
| if _export_onnx_opset_version < 9: |
| not is_registered_op(op[0], domain, version): | ||
| register_op(op[0], op[1], domain, version) | ||
| iter_version = iter_version - 1 | ||
| while iter_version != 9: |
There was a problem hiding this comment.
I would suggest not registering op set 9 everywhere.
Summary: This is an extension to the original PR #21765 1. Increase the coverage of different opsets support, comments, and blacklisting. 2. Adding backend tests for both caffe2 and onnxruntime on opset 7 and opset 8. 3. Reusing onnx model tests in caffe2 for onnxruntime. Pull Request resolved: #22421 Reviewed By: zrphercule Differential Revision: D16225518 Pulled By: houseroad fbshipit-source-id: 01ae3eed85111a83a0124e9e95512b80109d6aee
|
#22421 has been merged already. So close this PR. |
8 participants
No description provided.