Skip to content

Fix _thnn_fused_lstm_cell backward#11872

Closed
t-vi wants to merge 3 commits intopytorch:masterfrom
t-vi:lstm_backward
Closed

Fix _thnn_fused_lstm_cell backward#11872
t-vi wants to merge 3 commits intopytorch:masterfrom
t-vi:lstm_backward

Conversation

@t-vi
Copy link
Collaborator

@t-vi t-vi commented Sep 19, 2018

There are two parts:

  • Optional tensors cannot be dispatch tensors because dispatch
    tensors cannot be optional.
  • While the kernel dealt with undefined grad_outs, the logistics
    around it did not fully accomodate grad_hy being undefined.

Fixes: #11800

Thank you, @mttk for the reproduction!

@t-vi
Fix _thnn_fused_lstm_cell backward
2c5140b 
There are two parts:
- Optional tensors cannot be dispatch tensors because dispatch
  tensors cannot be optional.
- While the kernel dealt with undefined grad_outs, the logistics
  around it did not fully accomodate grad_hy being undefined.

Fixes: pytorch#11800

Thank you, @mttk for the reproduction!
@t-vi
Copy link
Collaborator Author

t-vi commented Sep 19, 2018 •
edited
Loading

Needless to say, I'd be happy to split the PR into two parts if you think they're too unrelated.
There aren't any other functions affected, but there is this comment that would now be obsolete:

# NB: We MUST call the input self, otherwise codegen will attempt to
# dispatch on ggI... which might be undefined.
- func: _convolution_double_backward(Tensor? ggI, Tensor? ggW, Tensor? ggb, Tensor gO, Tensor weight, Tensor self, IntList stride, IntList padding, IntList dilation, bool transposed, IntList output_padding, int64_t groups, bool benchmark, bool deterministic, bool cudnn_enabled, std::array<bool,3> output_mask) -> (Tensor, Tensor, Tensor)

@ssnl
Copy link
Collaborator

ssnl commented Sep 19, 2018

Nice! Is it possible to add a test for this?

@t-vi
Copy link
Collaborator Author

t-vi commented Sep 19, 2018 •
edited
Loading

how about torch.autograd.gradcheck(lambda *x: torch.lstm_cell(*x)[1], (data[:,0,:], h0, w_ih, w_hh, b_ih, b_hh))

I'll add that, but one question: How fixed is the torch.lstm_cell interface here and should I be concerned about that? I'll use the LSTMCell module and double, that should back to the right function.

ssnl
ssnl reviewed Sep 19, 2018
View reviewed changes
@ssnl
Copy link
Collaborator

ssnl commented Sep 19, 2018

Feel free to delete the comment above _convolution_double_backward :)

@t-vi
Copy link
Collaborator Author

t-vi commented Sep 20, 2018

The failure is:

_____________ TestCaffe2End2End.test_bvlc_reference_rcnn_ilsvrc13 ______________
20:53:09 
20:53:09 self = <caffe2.python.onnx.tests.c2_ref_test.TestCaffe2End2End testMethod=test_bvlc_reference_rcnn_ilsvrc13>
20:53:09 
20:53:09     @unittest.skipIf("IN_CIRCLECI" in os.environ, "FIXME: flaky test in CircleCI")
20:53:09     def test_bvlc_reference_rcnn_ilsvrc13(self):
20:53:09 >       self._test_net('bvlc_reference_rcnn_ilsvrc13')

I'd say not just in CircleCI...

apaszke
apaszke approved these changes Sep 20, 2018
View reviewed changes
facebook-github-bot
facebook-github-bot reviewed Sep 20, 2018
View reviewed changes
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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

zdevito pushed a commit to zdevito/ATen that referenced this pull request Sep 21, 2018
@t-vi @facebook-github-bot
Fix _thnn_fused_lstm_cell backward (#11872)
5495141 
Summary:
There are two parts:
- Optional tensors cannot be dispatch tensors because dispatch
  tensors cannot be optional.
- While the kernel dealt with undefined grad_outs, the logistics
  around it did not fully accomodate grad_hy being undefined.

Fixes: #11800

Thank you, mttk for the reproduction!
Pull Request resolved: pytorch/pytorch#11872

Differential Revision: D9978527

Pulled By: apaszke

fbshipit-source-id: e622c288d2eac93bd8388e141fb773f2588e2b8f
iotamudelta pushed a commit to ROCm/pytorch that referenced this pull request Sep 21, 2018
@t-vi @iotamudelta
Fix _thnn_fused_lstm_cell backward (pytorch#11872)
6e36873 
Summary:
There are two parts:
- Optional tensors cannot be dispatch tensors because dispatch
  tensors cannot be optional.
- While the kernel dealt with undefined grad_outs, the logistics
  around it did not fully accomodate grad_hy being undefined.

Fixes: pytorch#11800

Thank you, mttk for the reproduction!
Pull Request resolved: pytorch#11872

Differential Revision: D9978527

Pulled By: apaszke

fbshipit-source-id: e622c288d2eac93bd8388e141fb773f2588e2b8f
@ssnl ssnl mentioned this pull request Sep 22, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ssnl ssnl ssnl left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+1 more reviewer

@apaszke apaszke apaszke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"undefined Tensor" error in backward

5 participants

@t-vi @ssnl @apaszke @facebook-github-bot @ezyang