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
Symintifying slice ops #85196
Symintifying slice ops #85196
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85196
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 698e8ef: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ghstack-source-id: c29710e68c31b61d63087d5a28bfd68e0f3beed6 Pull Request resolved: #85196
Testing? |
[ghstack-poisoned]
ghstack-source-id: c385820d98ff186b0ba40e3bc25ead4e3caab508 Pull Request resolved: #85196
[ghstack-poisoned]
ghstack-source-id: ad285f4c3841cd043d6046728e6e9cc579cf5e1e Pull Request resolved: #85196
[ghstack-poisoned]
ghstack-source-id: 50879dfed96d3bc0d9e7a0157702f78e2d2ab958 Pull Request resolved: #85196
@@ -175,7 +176,8 @@ def get_name(op): | |||
torch._lazy.mark_step() | |||
torch._lazy.wait_device_ops() | |||
prefix = "aten" if op.name in FALLBACK_LIST else "lazy" | |||
found = f"{prefix}::{op.name}" in remove_suffixes(torch._lazy.metrics.counter_names()) | |||
symint_suffix = "_symint" if op.name in HAS_SYMINT_SUFFIX else "" | |||
found = f"{prefix}::{op.name}{symint_suffix}" in remove_suffixes(torch._lazy.metrics.counter_names()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ghstack-poisoned]
ghstack-source-id: 2a2518e3c43a34db7a19386a8538093ec6bac249 Pull Request resolved: #85196
[ghstack-poisoned]
ghstack-source-id: 13554fb4ff2a894fa1bca9b6bc393db4a6d799fb Pull Request resolved: #85196
[ghstack-poisoned]
[ghstack-poisoned]
test/test_decomp.py
Outdated
]: | ||
yield i | ||
slice = SliceOpInfo() | ||
self.do_cross_ref('cpu', torch.float, slice, run_all=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weren't you going to move this into common_methods_invocations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it blows up spectacularly in #85314. I'd suggest we do it as a follow up if you don't mind. I'm usually pretty good at following up, lol :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but... like... that the OpInfo isn't working with the other tests implies that the OpInfo is wrong...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but... like... that the OpInfo isn't working with the other tests implies that the OpInfo is wrong...
It seems it just needs way more integration work to run all the tests we run with OpInfos
- there are issues using torch.ops.aten.slice as
OpInfo.op
- it actually passes for more types than I specified
- it also passes more forward/backward tests than I specified it should be passing.
For testing the forward of slice
which I think was the main intent of Will's tests, it works pretty well as the part of decomp
tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is going to be that hard to fix. I'm checking out your PR to try it.
torch/_decomp/decompositions.py
Outdated
@@ -13,6 +14,8 @@ | |||
from torch._prims_common.wrappers import out_wrapper | |||
from torch.utils._pytree import tree_flatten, tree_map | |||
|
|||
old_slice = slice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ded
[ghstack-poisoned]
[ghstack-poisoned]
ghstack-source-id: cac74891e54d687d10138ff63127c34021c27b20 Pull Request resolved: #85196 slice tests rename slice to avoid hijacking builtins.slice fix lintrunner remove dead code stuff fix bbrks
[ghstack-poisoned]
ghstack-source-id: 886e4bd49a09b74c949e779f547267ccfe31be87 Pull Request resolved: #85196 slice tests rename slice to avoid hijacking builtins.slice fix lintrunner remove dead code stuff fix bbrks
@pytorchbot merge |
@pytorchbot successfully started a merge job. Check the current status here. |
Hey @Krovatkin. |
@@ -609,6 +610,58 @@ def slice_backward( | |||
return torch.slice_scatter(grad_input, grad_output, dim, start, end, step) | |||
|
|||
|
|||
@register_decomposition(aten.slice.Tensor) | |||
def slice_forward( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dumb q: why do we need a python decomp for slice
if we're already c10::SymInt
-ifying the C++ implementation? Is the python impl still required for something? (either way, I totally see a python impl being useful to have).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look over the patch carefully. We did not, in fact, SymInt'ify the C++ implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
welp, thanks. Needed to scan the codegen again to remember how it works: If we put _symint
at the end of the kernel name in native_functions.yaml
, then the codegen will expect you to symint-ify the C++ kernel
@pytorchmergebot revert -m "Break internal build Exutorch" -c ghfirst |
@pytorchbot successfully started a revert job. Check the current status here. |
@Krovatkin your PR has been successfully reverted. |
This reverts commit 4c01c51. Reverted #85196 on behalf of https://github.com/atalman due to Break internal build Exutorch
This reverts commit 3a171df. [ghstack-poisoned]
This reverts commit 3a171df. [ghstack-poisoned]
This reverts commit 3a171df. Pull Request resolved: #85746 Approved by: https://github.com/albanD
This reverts commit 3a171df. Pull Request resolved: pytorch#85746 Approved by: https://github.com/albanD
Pull Request resolved: #85196 Approved by: https://github.com/ezyang
This reverts commit 4c01c51. Reverted #85196 on behalf of https://github.com/atalman due to Break internal build Exutorch
This reverts commit 3a171df. Pull Request resolved: #85746 Approved by: https://github.com/albanD
Pull Request resolved: pytorch#85196 Approved by: https://github.com/ezyang
This reverts commit 4c01c51. Reverted pytorch#85196 on behalf of https://github.com/atalman due to Break internal build Exutorch
This reverts commit 3a171df. Pull Request resolved: pytorch#85746 Approved by: https://github.com/albanD
Stack from ghstack (oldest at bottom):