Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ONNX export of RNNs with no bias #36894

Closed
wants to merge 9 commits into from
Closed

Conversation

@yaeldekel
Copy link
Contributor

@yaeldekel yaeldekel commented Apr 19, 2020

Fixes #34800 .

Currently, the LSTM/RNN/GRU export to ONNX can't handle models without a bias term.

@dr-ci
Copy link

@dr-ci dr-ci bot commented Apr 19, 2020

💊 CI failures summary and remediations

As 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.

See how this bot performed.

This comment has been revised 40 times.

Loading

@albanD
Copy link
Contributor

@albanD albanD commented Apr 20, 2020

@houseroad you know who would be best to review this PR?

Loading

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

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

Loading

@neginraoof
Copy link
Collaborator

@neginraoof neginraoof commented May 14, 2020

cc @houseroad for review. Thanks!

Loading

@yaeldekel yaeldekel force-pushed the lstm branch 2 times, most recently from c730f15 to c2e2b89 May 19, 2020
@yaeldekel
Copy link
Contributor Author

@yaeldekel yaeldekel commented May 19, 2020

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
13:31:23 test_reduce (main.TestNCCL) ... FAIL
13:31:23 test_reduce_scatter (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?
Thanks!


In reply to: 628921418 [](ancestors = 628921418)

Loading

@neginraoof
Copy link
Collaborator

@neginraoof neginraoof commented May 26, 2020

cc @houseroad for review. Thanks!

Loading

1 similar comment
@yaeldekel
Copy link
Contributor Author

@yaeldekel yaeldekel commented Jun 1, 2020

cc @houseroad for review. Thanks!

Loading

@houseroad
Copy link
Member

@houseroad houseroad commented Jun 1, 2020

Thanks for pinging, doing it now :-)

Loading

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

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

Loading

Copy link
Member

@houseroad houseroad left a comment

Looks good, thanks. Could you rebase to master and trigger the tests? The current pr is a bit old.

Loading

input = tuple(inputs)
return input

input = make_input(RNN_BATCH_SIZE, 1, 0)
Copy link
Member

@houseroad houseroad Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ditto

Loading

h0 = torch.randn(num_layers * num_directions, batch_size, hidden_size)
return model, (input, h0)

batch_size1 = 3
Copy link
Member

@houseroad houseroad Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ditto.

Loading

c0 = torch.randn(num_layers * num_directions, BATCH_SIZE, RNN_HIDDEN_SIZE)
return model, (input, (h0, c0))

model1, input1 = get_LstmNet_model_and_inputs(1, True)
Copy link
Member

@houseroad houseroad Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@yaeldekel
Copy link
Contributor Author

@yaeldekel yaeldekel commented Jun 2, 2020

Thank you!


In reply to: 637018282 [](ancestors = 637018282)

Loading

@yaeldekel
Copy link
Contributor Author

@yaeldekel yaeldekel commented Jun 8, 2020

Hi @houseroad , I have addressed your comments, and all the builds are passing. Would you like additional changes or can this PR be merged?
Thanks!

Loading

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

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

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Jun 9, 2020

@houseroad merged this pull request in 0251ba6.

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Jun 9, 2020

@houseroad merged this pull request in 0251ba6.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants