-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Updates alias pattern (and torch.absolute to use it) #42586
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
Conversation
💊 CI failures summary and remediationsAs of commit db2df72 (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 or post in the (internal) Dr. CI Users group. This comment has been revised 4 times. |
bhosmer
left a comment
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 LGTM, seems like a viable near-term approach (and preferable to the "Language Pattern"). (Also I think the "drift" con is actually mitigated by the redispatch, since that will fail if the callee function's signature is no longer compatible.)
adding @ezyang for a quick reality check in case I'm missing any subtler consequences
aten/src/ATen/native/UnaryOps.cpp
Outdated
| Tensor absolute(const Tensor& self) { | ||
| return self.abs(); | ||
| } | ||
| Tensor& absolute_(Tensor& self) { return self.abs_(); } |
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.
nit: maybe break this into 3 lines like above?
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.
Will do!
ezyang
left a comment
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 seems fine as a stopgap. Note that this particular PR is BC breaking for backend extenders as they can no longer override absolute in the same way they were able to do before. Make sure XLA tests are still OK.
facebook-github-bot
left a comment
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.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This PR canonicalizes our (current) pattern for adding aliases to PyTorch. That pattern is:
An alternative pattern would be to use Python and C++ language features to alias ops directly. For example in Python:
Let the pattern in this PR be the "native function" pattern, and the alternative pattern be the "language pattern." There are pros/cons to both approaches:
Pros of the "Language Pattern"
Cons of the "Language Pattern"
Pros of the "Native Function" pattern
Cons of the "Native Function" pattern
While either approach is reasonable, I suggest the "native function" pattern since it preserves "native_functions.yaml" as a source of truth and minimizes the number of alias lists that need to be maintained. In the future, entries in native_functions.yaml may support an "alias" argument and replace whatever pattern we choose now.
Ops that are likely to use aliasing are: