symintify all of derivatives.yaml#86610
symintify all of derivatives.yaml#86610bdhirsh wants to merge 15 commits intogh/bdhirsh/324/basefrom
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/86610
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit bfc235d: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
| // The below ops don't get a duplicated C++ implementation. | ||
| // They are backward ops, which make them very unlikely to be called directly | ||
| // by external code (at::native::trace_backward). | ||
| // They get their own declaration for BC purposes however. |
There was a problem hiding this comment.
@albanD @wconstab wondering if anyone has thoughts on this. Keeping these extra ops for BC to guarantee no internal failures felt like a good idea. But instead of duplicating all of the code, I just routed them to the symint variants (cuts down on code size a bunch but is marginally slower - shouldn't really matter in practice though, since hopefully nobody is calling these).
| #include <c10/core/QScheme.h> | ||
| #include <ATen/core/Reduction.h> | ||
| #include <ATen/core/Tensor.h> | ||
| #include <ATen/native/NonSymbolicBC.h> |
There was a problem hiding this comment.
The other change to minimize (guarantee?) no internal breakages is here - fbcode uses NativeFunctions.h internally, so they should get the headers for the BC symbols automatically.
albanD
left a comment
There was a problem hiding this comment.
Sounds good!
Hopefully it lands cleanly!
|
|
||
| #include <ATen/ATen.h> | ||
| #include <ATen/NestedTensorImpl.h> | ||
| #include <ATen/native/NonSymbolicBC.h> |
There was a problem hiding this comment.
I though we said that we might as well add this to the general Functions header?
There was a problem hiding this comment.
I added them to the general NativeFunctions.h header (since they're part of at::native::) For the OSS builds though, that won't help because we use per-operator headers - so we still need to manually include them here.
That feels a bit weird, but should still accomplish the goal of reducing the risk of internal build failures.
There was a problem hiding this comment.
although - sigh - OSS mobile builds aren't recognizing ATen/native/NonSymbolicBC.h. I probably need to add it to a makefile somewhere.
There was a problem hiding this comment.
hmm @albanD - it looks like the build system is complaining because I'm trying to include a header from aten/native (ATen/native/NonSymbolicBC.h) inside of a header from plain ATen (ATen/NativeFunctions.h).
I think that logically, NonSymbolicBC.h and NativeFunctions.h should probably live in the same location, since they both refer to native variants of the aten API. That makes me think that NonSymbolicBC.h should be moved to live in aten/src/ATen instead of aten/src/ATen/native.
The whole point of the changes in this PR though were to reduce the risk of internal changes being needed, and that would definitely require an internal change, since that header is used internally now 😛.
So my current plan is just to ignore NonSymbolicBC.h entirely in this PR, since it's very unlikely that any of the at::native::*_backward() functions that I'm changing are actually directly called anywhere. LMK what you think!
Big-bang PR to symintify **all** .sizes() calls in derivatives.yaml, which will be needed for symbolic tracing. * with the exception of `split()`, which is tougher to land because it requires internal changes. [ghstack-poisoned]
Big-bang PR to symintify **all** .sizes() calls in derivatives.yaml, which will be needed for symbolic tracing. * with the exception of `split()`, which is tougher to land because it requires internal changes. [ghstack-poisoned]
Big-bang PR to symintify **all** .sizes() calls in derivatives.yaml, which will be needed for symbolic tracing. * with the exception of `split()`, which is tougher to land because it requires internal changes. [ghstack-poisoned]
|
@pytorchbot merge -g |
Merge startedYour change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
Hey @bdhirsh. |
ghstack-source-id: 85d6c20 Pull Request resolved: pytorch/pytorch#86610
Big-bang PR to symintify all .sizes() calls in derivatives.yaml, which will be needed for symbolic tracing.
split(), which is tougher to land because it requires internal changes.Stack from ghstack (oldest at bottom):