| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 24.8k
[BE] Make maybe_aliasing_or_mutating proper tag #131990
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
[BE] Make maybe_aliasing_or_mutating proper tag #131990
Conversation
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) [ghstack-poisoned]
tugsbayasgalan
requested review from
avikchaudhuri,
zhxchen17,
ydwu4,
angelayi and
ezyang
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/131990
Note: Links to docs will display an error until the docs builds have been completed. âś… No FailuresAs of commit bc3ffca with merge base a847790 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D60347117 |
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) ghstack-source-id: 235522623 Pull Request resolved: #131990
| @@ -4307,6 +4315,7 @@ | |||
| CUDA: batch_norm_cuda | |||
| MPS: batch_norm_mps | |||
| MkldnnCPU: mkldnn_batch_norm | |||
| tags: maybe_aliasing_or_mutating | |||
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'm not sure about this, in these cases the schema is just wrong (as opposed to composites, where schema is not meaningful)
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.
Do you mean we don't need this tag at all or we need two seperate tags (one for CIA ops that are maybe mutating and one for primitive ops)?
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 new tag should probably only apply to CIA ops, since those ops are "allowed" to maybe-alias/mutate.
Non-CIA ops that maybe alias/mutate will just give silently wrong results today when run with PT2 (and autograd), so we should probably just fix them instead of giving them a dedicated tag. And and at this point, native_batch_norm is "fixed" in-so-far as we never see the op in graphs we trace anymore, since we see the _native_batch_norm_legit op instead (although @andrewor14 is working on consolidating batch norm ops here #119496)
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D60347117 |
Pull Request resolved: #131990 For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) ghstack-source-id: 236160755
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) [ghstack-poisoned]
|
@tugsbayasgalan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. We need to special case native_batch_norm because it is not a CIA but has a wrong schema. I guess native_batch_norm will be removed at some point, so until then we just keep it around. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) [ghstack-poisoned]
Pull Request resolved: #131990 For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) ghstack-source-id: f97cf8f
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. We need to special case native_batch_norm because it is not a CIA but has a wrong schema. I guess native_batch_norm will be removed at some point, so until then we just keep it around. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) [ghstack-poisoned]
Pull Request resolved: #131990 For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) ghstack-source-id: d75bae5
|
@tugsbayasgalan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
|
||
| # We force create native_batch_norm because the below materialization logic | ||
| # only applies to CIA ops. | ||
| maybe_aliasing_or_mutating_ops = [torch.ops.aten.native_batch_norm.default] |
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 looks like you're hardcoding aten.native_batch_norm in two places: both here, and in your helper _should_decompose_because_unsafe_op(op). Can't you just re-use that _should_decompose_because_unsafe_op() helper in ordre to generate this list?
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.
left some comments!
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Merge failedReason: This PR has internal changes and must be landed via Phabricator! Please try reimporting/rexporting the PR! Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge |
Merge failedReason: This PR has internal changes and must be landed via Phabricator! Please try reimporting/rexporting the PR! Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 jobs have failed, first few of them are: Meta Internal-Only Changes Check Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -i |
Merge startedYour change will be merged while ignoring the following 1 checks: Meta Internal-Only Changes Check Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. We need to special case native_batch_norm because it is not a CIA but has a wrong schema. I guess native_batch_norm will be removed at some point, so until then we just keep it around. D60347117 Pull Request resolved: pytorch#131990 Approved by: https://github.com/bdhirsh
Pull Request resolved: pytorch/pytorch#131990 For better tracking, we need to make maybe aliasing/mutating ops with proper tag. Differential Revision: [D60347117](https://our.internmc.facebook.com/intern/diff/D60347117/) ghstack-source-id: 58f8c2f
Stack from ghstack (oldest at bottom):
For better tracking, we need to make maybe aliasing/mutating ops with proper tag. We need to special case native_batch_norm because it is not a CIA but has a wrong schema. I guess native_batch_norm will be removed at some point, so until then we just keep it around.
D60347117