SymIntify roi_align #7448
SymIntify roi_align #7448
Conversation
Signed-off-by: Edward Z. Yang <ezyang@meta.com>
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7448
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Signed-off-by: Edward Z. Yang <ezyang@meta.com>
There was a problem hiding this comment.
Thanks @ezyang, I just have questions at this point, nothing substantial. LMK if you need a stamp on the PR, but also feel free to merge without one if this is urgent
| } // namespace detail | ||
|
|
||
| TORCH_LIBRARY_FRAGMENT(torchvision, m) { | ||
| m.def(TORCH_SELECTIVE_SCHEMA( | ||
| "torchvision::roi_align(Tensor input, Tensor rois, float spatial_scale, int pooled_height, int pooled_width, int sampling_ratio, bool aligned) -> Tensor")); | ||
| "torchvision::roi_align(Tensor input, Tensor rois, float spatial_scale, SymInt pooled_height, SymInt pooled_width, int sampling_ratio, bool aligned) -> Tensor")); |
There was a problem hiding this comment.
Since we're now only registering the SymInt signature, do we still need to keep the pre-existing roi_align definitions/declarations that are using int64_t?
There was a problem hiding this comment.
In core, we had to keep the old signatures because the C++ API is public API, and the SymInt signature is not exactly interchangeable with the int signature (as it can affect what implicit conversions are specified). If your old signatures are not public API, we can remove them too, but I'm guessing they are public-ish? In any case, this is the most conservative change.
There was a problem hiding this comment.
I'm open to other strategies, as we will have to do this for every function we SymInt'ify which is going to be a bit of a pain.
There was a problem hiding this comment.
Got it, thanks.
I'm guessing they are public-ish
Yeah... They're in the "we want them to be private but we don't know who's using them in the wild" category.
Let's keep them in for now and perhaps reconsider if this becomes too much of a mess.
torchvision/_meta_registrations.py
Outdated
There was a problem hiding this comment.
Could you explain why this file is needed, or point me to a place where I can read on it?
There was a problem hiding this comment.
This is analogous to torch/_meta_registrations.py. https://docs.google.com/document/d/1GgvOe7C8_NVOMLOCwDaYV1mXXyHMXY7ExoewHqooxrs/edit#heading=h.fh8zzonyw8ng goes into what the Python meta function workflow looks like. But the basic idea is that if we write the meta functions in Python, there needs to be some flow where we can tell the C++ dispatcher about the meta function. That's the registration file here.
There was a problem hiding this comment.
Thanks @ezyang , approving now to unblock. Do we want to add some tests before merging, or do that later?
| } // namespace detail | ||
|
|
||
| TORCH_LIBRARY_FRAGMENT(torchvision, m) { | ||
| m.def(TORCH_SELECTIVE_SCHEMA( | ||
| "torchvision::roi_align(Tensor input, Tensor rois, float spatial_scale, int pooled_height, int pooled_width, int sampling_ratio, bool aligned) -> Tensor")); | ||
| "torchvision::roi_align(Tensor input, Tensor rois, float spatial_scale, SymInt pooled_height, SymInt pooled_width, int sampling_ratio, bool aligned) -> Tensor")); |
There was a problem hiding this comment.
Got it, thanks.
I'm guessing they are public-ish
Yeah... They're in the "we want them to be private but we don't know who's using them in the wild" category.
Let's keep them in for now and perhaps reconsider if this becomes too much of a mess.
|
@NicolasHug I'm going to try to stand up some sort of shared testing infrastructure across the repos to do it, so yeah, separate PR would be appreciated |
|
Hey @ezyang! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
after pytorch/vision#7448, the test is now ok Pull Request resolved: #97706 Approved by: https://github.com/tugsbayasgalan
This reverts commit 8f2e5c9.
Summary: Signed-off-by: Edward Z. Yang <ezyang@meta.com> Reviewed By: vmoens Differential Revision: D44552412 fbshipit-source-id: 05ce4c02d0cbb7a02bdbf4ec68f13bf2056b46c3 Co-authored-by: Philip Meier <github.pmeier@posteo.de> Co-authored-by: Nicolas Hug <nh.nicolas.hug@gmail.com>
This is one step in making it possible to trace through roi_align in PT2 with dynamic=True; the other missing piece is a meta function.
The duplication of the C++ signatures is a bit of a chore. In PyTorch core we have a codegen that can handle it, but we don't have that here unfortunately.
Signed-off-by: Edward Z. Yang ezyang@meta.com