add meta for segment_reduce_backward #124988
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/124988
Note: Links to docs will display an error until the docs builds have been completed. ❌ 13 New FailuresAs of commit cc38e38 with merge base e04c7b1 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
github-actions
bot
requested review from
SherlockNoMad,
albanD,
antoniojkim,
ezyang and
miladm
torch/_meta_registrations.py
Outdated
| @register_meta(aten._segment_reduce_backward) | ||
| @out_wrapper() | ||
| def meta__segment_reduce_backward(grad, output, data, reduce, lengths=None, offsets=None, axis=0, initial=None): | ||
| assert lengths is not None or offsets is not None, "segment_reduce(): Either lengths or offsets must be defined" |
as titled, wrote by staring at the cpu kernel [ghstack-poisoned]
| self.assertEqual(res.dtype, torch.float32) | ||
| self.assertEqual(res.untyped_storage().data_ptr(), 0) | ||
|
|
||
| def test_segment_reduce_backward(self): |
There was a problem hiding this comment.
Don't we have opinfo based test for these. This will give us much better coverage for inputs
There was a problem hiding this comment.
I saw an opinfo for the forward, but not the backward.
Lmk if you agree - it seems like we could either:
(1) Add a dedicated OpInfo for the backward op
(2) Add some generic testing in test_meta.py that loops over every existing OpInfo with meta tensors and invokes backward
(2) would be nice, but I'm not sure I'll have time for it. Would you prefer I add a new opinfo over the one-off test? (totally happy to, agreed the one-off test isn't great)
There was a problem hiding this comment.
I think a new opinfo is the right way to go tbh. We want all other tests on native op to run on the backward!
And you should be able to re-use the sample function from the forward mostly
| data_contig = data.contiguous() | ||
| grad_contig = grad.contiguous() |
There was a problem hiding this comment.
Do we actually read the updates below?
Also this behavior is definitely not covered by the test!
There was a problem hiding this comment.
yeah it's definitely not
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
This can be written more idiomatically but I'm going ahead and stamping
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
reland of #124988 [ghstack-poisoned]
reland of #124988 Pull Request resolved: #137442 Approved by: https://github.com/albanD
4 participants
as titled, wrote by staring at the cpu kernel
Stack from ghstack (oldest at bottom):