Fix ONNX export of RNNs with no bias#36894
Conversation
💊 CI failures summary and remediationsAs of commit 7d49669 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 40 times. |
|
@houseroad you know who would be best to review this PR? |
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.
c730f15 to
c2e2b89
Compare
|
Hi @houseroad, I am getting some test failures that seem unrelated to this change: in py3.6-clang7-rocmdeb-ubuntu16.04-test2: 13:31:23 test_broadcast (main.TestNCCL) ... FAIL in py3.6-clang7-rocmdeb-ubuntu16.04-test1: 2:56:42 test_softmax_results_cuda_float16 (main.TestNNDeviceTypeCUDA) ... FAIL and in pytorch_macos_10_13_py3_test: test_float_to_int_conversion_finite_cpu_int16 - TestTorchDeviceTypeCPU Do you know how to investigate these failures? In reply to: 628921418 [](ancestors = 628921418) |
|
cc @houseroad for review. Thanks! |
1 similar comment
|
cc @houseroad for review. Thanks! |
|
Thanks for pinging, doing it now :-) |
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
There was a problem hiding this comment.
Looks good, thanks. Could you rebase to master and trigger the tests? The current pr is a bit old.
There was a problem hiding this comment.
Nit: make the inputs as an array, and loop through the array. So we don't need to repeat get_LstmNet_model_and_inputs/run_test pattern so many times.
|
Thank you! In reply to: 637018282 [](ancestors = 637018282) |
|
Hi @houseroad , I have addressed your comments, and all the builds are passing. Would you like additional changes or can this PR be merged? |
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 merged this pull request in 0251ba6. |
|
@houseroad merged this pull request in 0251ba6. |
Summary: Fixes pytorch#34800 . Currently, the LSTM/RNN/GRU export to ONNX can't handle models without a bias term. Pull Request resolved: pytorch#36894 Reviewed By: hl475 Differential Revision: D21134794 Pulled By: houseroad fbshipit-source-id: e71e089025a3dc7e8c883ff99cd788c5f302492e
Fixes #34800 .
Currently, the LSTM/RNN/GRU export to ONNX can't handle models without a bias term.