Improve aten::backward handling#36750
Closed
smessmer wants to merge 4 commits intogh/smessmer/205/basefrom
Closed
Conversation
- It seems the JIT schema for aten::backward and the schema in native_functions.yaml diverged on whether the retain_graph/keep_graph parameter takes a `bool` or a `bool?`. Make them identical again. - Also remove the mutability annotation for the self parameter. This does not make sense together with AliasAnalysisKind::CONSERVATIVE and it triggered an assertion - After fixing the mutability annotation, we can fix that assertion so that it doesn't exclude aten::backward from its check anymore - Last but not least, remove the unboxed_only marker from aten::backward. This requires us to add a special case in register_c10_ops.cpp for it, because JIT still has its own implementation. Differential Revision: [D21004102](https://our.internmc.facebook.com/intern/diff/D21004102/) [ghstack-poisoned]
💊 Build failures summary and remediationsAs of commit 3b2da0a (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker. This comment has been revised 9 times. |
This was referenced Apr 16, 2020
- It seems the JIT schema for aten::backward and the schema in native_functions.yaml diverged on whether the retain_graph/keep_graph parameter takes a `bool` or a `bool?`. Make them identical again. - Also remove the mutability annotation for the self parameter. This does not make sense together with AliasAnalysisKind::CONSERVATIVE and it triggered an assertion - After fixing the mutability annotation, we can fix that assertion so that it doesn't exclude aten::backward from its check anymore - Last but not least, remove the unboxed_only marker from aten::backward. This requires us to add a special case in register_c10_ops.cpp for it, because JIT still has its own implementation. Differential Revision: [D21004102](https://our.internmc.facebook.com/intern/diff/D21004102/) [ghstack-poisoned]
bhosmer
approved these changes
Apr 16, 2020
bhosmer
left a comment
There was a problem hiding this comment.
LG mod tests :)
Maybe add a sentence summarizing the post-PR end state to the description? the changes make sense but a recap of the "improved" situation might be good
| // create_graph=True so we use aliasAnalysisConservative for these two OPs | ||
| Operator( | ||
| "aten::backward(Tensor[](a!) tensors, Tensor?[]? grad_tensors=None, bool? retain_graph=None, bool create_graph=False) -> ()", | ||
| "aten::backward.TensorList(Tensor[] tensors, Tensor?[]? grad_tensors=None, bool? retain_graph=None, bool create_graph=False) -> ()", |
There was a problem hiding this comment.
This was name duplicated before? It's serialization of calls to the unlucky variant that would break, right?
Contributor
Author
There was a problem hiding this comment.
yes but that serialization only happens for the lite interpreter where we don't have bc concerns for models doing autograd yet.
- It seems the JIT schema for aten::backward and the schema in native_functions.yaml diverged on whether the retain_graph/keep_graph parameter takes a `bool` or a `bool?`. Make them identical again. - Also remove the mutability annotation for the self parameter. This does not make sense together with AliasAnalysisKind::CONSERVATIVE and it triggered an assertion - After fixing the mutability annotation, we can fix that assertion so that it doesn't exclude aten::backward from its check anymore - Last but not least, remove the unboxed_only marker from aten::backward. This requires us to add a special case in register_c10_ops.cpp for it, because JIT still has its own implementation. After this PR, we still have separate implementations for aten::backward for JIT (in register_prim_ops_fulljit.cpp) and for eager mode (in native_functions.yaml), but they share the same signature again. Differential Revision: [D21004102](https://our.internmc.facebook.com/intern/diff/D21004102/) [ghstack-poisoned]
smessmer
added a commit
that referenced
this pull request
Apr 16, 2020
Pull Request resolved: #36750 - It seems the JIT schema for aten::backward and the schema in native_functions.yaml diverged on whether the retain_graph/keep_graph parameter takes a `bool` or a `bool?`. Make them identical again. - Also remove the mutability annotation for the self parameter. This does not make sense together with AliasAnalysisKind::CONSERVATIVE and it triggered an assertion - After fixing the mutability annotation, we can fix that assertion so that it doesn't exclude aten::backward from its check anymore - Last but not least, remove the unboxed_only marker from aten::backward. This requires us to add a special case in register_c10_ops.cpp for it, because JIT still has its own implementation. ghstack-source-id: 102324462 Differential Revision: [D21004102](https://our.internmc.facebook.com/intern/diff/D21004102/)
- It seems the JIT schema for aten::backward and the schema in native_functions.yaml diverged on whether the retain_graph/keep_graph parameter takes a `bool` or a `bool?`. Make them identical again. - Also remove the mutability annotation for the self parameter. This does not make sense together with AliasAnalysisKind::CONSERVATIVE and it triggered an assertion - After fixing the mutability annotation, we can fix that assertion so that it doesn't exclude aten::backward from its check anymore - Last but not least, remove the unboxed_only marker from aten::backward. This requires us to add a special case in register_c10_ops.cpp for it, because JIT still has its own implementation. After this PR, we still have separate implementations for aten::backward for JIT (in register_prim_ops_fulljit.cpp) and for eager mode (in native_functions.yaml), but they share the same signature again. Differential Revision: [D21004102](https://our.internmc.facebook.com/intern/diff/D21004102/) [ghstack-poisoned]
smessmer
added a commit
that referenced
this pull request
Apr 17, 2020
Pull Request resolved: #36750 - It seems the JIT schema for aten::backward and the schema in native_functions.yaml diverged on whether the retain_graph/keep_graph parameter takes a `bool` or a `bool?`. Make them identical again. - Also remove the mutability annotation for the self parameter. This does not make sense together with AliasAnalysisKind::CONSERVATIVE and it triggered an assertion - After fixing the mutability annotation, we can fix that assertion so that it doesn't exclude aten::backward from its check anymore - Last but not least, remove the unboxed_only marker from aten::backward. This requires us to add a special case in register_c10_ops.cpp for it, because JIT still has its own implementation. ghstack-source-id: 102351871 Differential Revision: [D21004102](https://our.internmc.facebook.com/intern/diff/D21004102/)
Contributor
|
This pull request has been merged in 31f91d6. |
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 24, 2026
Summary: Pull Request resolved: pytorch#36750 - It seems the JIT schema for aten::backward and the schema in native_functions.yaml diverged on whether the retain_graph/keep_graph parameter takes a `bool` or a `bool?`. Make them identical again. - Also remove the mutability annotation for the self parameter. This does not make sense together with AliasAnalysisKind::CONSERVATIVE and it triggered an assertion - After fixing the mutability annotation, we can fix that assertion so that it doesn't exclude aten::backward from its check anymore - Last but not least, remove the unboxed_only marker from aten::backward. This requires us to add a special case in register_c10_ops.cpp for it, because JIT still has its own implementation. ghstack-source-id: 102351871 Test Plan: waitforsandcastle Differential Revision: D21004102 fbshipit-source-id: 19bd1adbd8103c214d32e5126671a809adec581e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
boolor abool?. Make them identical again.After this PR, we still have separate implementations for aten::backward for JIT (in register_prim_ops_fulljit.cpp) and for eager mode (in native_functions.yaml), but they share the same signature again.
Differential Revision: D21004102