Port roi_align to actually use dispatcher#2366
Conversation
This is still registering everything as catchalls, so we're really just moving deck chairs around, but payoff is coming soon. Signed-off-by: Edward Z. Yang <ezyang@fb.com>
3ce660f to
a0e6f91
Compare
|
CUDA is not done yet, needs similar treatment. |
a0e6f91 to
ad105f1
Compare
out of date? Cuda ops look ok/symmetric with CPU ops. Diffs make sense given what we discussed on slack. I think it will be straightforward to add an autocast layer as well, which will cast inputs then redispatch to one of the dispatch nexuses ( If this is mergeable, I can wait for merge then pull it to add an autocast layer and flesh out other ops similarly. |
| ctx->saved_data["input_shape"] = input.sizes(); | ||
| ctx->save_for_backward({rois}); | ||
| auto result = ROIAlign_forward( | ||
| at::AutoNonVariableTypeMode g; |
There was a problem hiding this comment.
Why an explicit autograd-disabling guard here? Does torch::autograd::Function not disable autograd automatically around forward?
The pre-PR forward doesn't use an explicit guard, and afaik Python-side torch.autograd.Function does disable autograd around its forward method. Both of these lead me to expect torch::autograd::Function also disables autograd around forward. (If it doesn't, I think it should. Aligning its behavior with the Python version makes sense to me. But that would be a Pytorch-side change.)
There was a problem hiding this comment.
fmassa
left a comment
There was a problem hiding this comment.
Thanks a lot for the port @ezyang !
Lint is failing though, can you run clang-format?
I'm ok merging this PR right now, just have a question about having to implement dummy gradgrad kernels, as we will probably be using this PR as a template for all other functions.
| }; | ||
|
|
||
| at::Tensor roi_align( | ||
| // TODO: There should be an easier way to do this |
There was a problem hiding this comment.
Can't we register a fallback kernel that raises an error on double-backward? So that we don't have to implement a dummy double-backwards kernel for all the ops.
There was a problem hiding this comment.
This would indeed be the right thing to do in core library. There will be some BC consequences though
You should probably put the casting utilities in a header file. We'll need to talk about what a good external facing API for it is. |
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
ad105f1 to
6155d0c
Compare
aye, right now I don't have a clear idea. Trying it for torchvision will help solidify what such an API needs. The fact that right now, autocast only wraps forward ops, and relies on autograd to reverse casts in backward, makes life easier. |
Codecov Report
@@ Coverage Diff @@
## master #2366 +/- ##
==========================================
+ Coverage 68.49% 69.86% +1.36%
==========================================
Files 93 93
Lines 7655 8361 +706
Branches 1177 1414 +237
==========================================
+ Hits 5243 5841 +598
- Misses 2075 2121 +46
- Partials 337 399 +62
Continue to review full report at Codecov.
|
* Switch torchvision registrations to new operator registration API. This is still registering everything as catchalls, so we're really just moving deck chairs around, but payoff is coming soon. Signed-off-by: Edward Z. Yang <ezyang@fb.com> * Port roi_align to actually use dispatcher Signed-off-by: Edward Z. Yang <ezyang@fb.com>
* Switch torchvision registrations to new operator registration API. This is still registering everything as catchalls, so we're really just moving deck chairs around, but payoff is coming soon. Signed-off-by: Edward Z. Yang <ezyang@fb.com> * Port roi_align to actually use dispatcher Signed-off-by: Edward Z. Yang <ezyang@fb.com> Co-authored-by: Edward Z. Yang <ezyang@fb.com>
No description provided.