Skip to content

Conversation

@mruberry
Copy link
Collaborator

@mruberry mruberry commented Aug 5, 2020

This PR canonicalizes our (current) pattern for adding aliases to PyTorch. That pattern is:

  • Copy the original functions native_functions.yaml entry, but replace the original function's name with their own.
  • Implement the corresponding functions and have them redispatch to the original function.
  • Add docstrings to the new functions that reference the original function.
  • Update the alias_map in torch/csrc/jit/passes/normalize_ops.cpp.
  • Update the op_alias_mappings in torch/testing/_internal/jit_utils.py.
  • Add a test validating the alias's behavior is the same as the original function's.

An alternative pattern would be to use Python and C++ language features to alias ops directly. For example in Python:

torch.absolute = torch.abs

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"

  • torch.absolute is torch.abs.
  • no (or very little) overhead for calling the alias.
  • no native_functions.yaml redundancy or possibility of "drift" between the original function's entries and the alias's.

Cons of the "Language Pattern"

  • requires manually adding doc entries
  • requires updating Python alias and C++ alias lists
  • requires hand writing alias methods on Tensor (technically this should require a C++ test to validate)
  • no single list of all PyTorch ops -- have to check native_functions.yaml and one of the separate alias lists

Pros of the "Native Function" pattern

  • alias declarations stay in native_functions.yaml
  • doc entries are written as normal

Cons of the "Native Function" pattern

  • aliases redispatch to the original functions
  • torch.absolute is not torch.abs (requires writing test to validate behavior)
  • possibility of drift between original's and alias's native_functions.yaml entries

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:

  • div (divide, true_divide)
  • mul (multiply)
  • bucketize (digitize)
  • cat (concatenate)
  • clamp (clip)
  • conj (conjugate)
  • rad2deg (degrees)
  • trunc (fix)
  • neg (negative)
  • deg2rad (radians)
  • round (rint)
  • acos (arccos)
  • acosh (arcosh)
  • asin (arcsin)
  • asinh (arcsinh)
  • atan (arctan)
  • atan2 (arctan2)
  • atanh (arctanh)
  • bartlett_window (bartlett)
  • hamming_window (hamming)
  • hann_window (hanning)
  • bitwise_not (invert)
  • gt (greater)
  • ge (greater_equal)
  • lt (less)
  • le (less_equal)
  • ne (not_equal)
  • ger (outer)

@mruberry mruberry changed the title Updates alias pattern (and absolute to use it) Updates alias pattern (and torch.absolute to use it) Aug 5, 2020
@mruberry mruberry requested review from bhosmer and eellison August 5, 2020 06:04
@mruberry mruberry requested a review from zou3519 August 5, 2020 06:08
@dr-ci
Copy link

dr-ci bot commented Aug 5, 2020

💊 CI failures summary and remediations

As 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.

See how this bot performed.

This comment has been revised 4 times.

Copy link

@bhosmer bhosmer left a 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

Tensor absolute(const Tensor& self) {
return self.abs();
}
Tensor& absolute_(Tensor& self) { return self.abs_(); }
Copy link

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@bhosmer bhosmer requested a review from ezyang August 5, 2020 21:59
Copy link
Contributor

@ezyang ezyang left a 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.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in 73642d9.

@mruberry mruberry deleted the alias_pattern branch August 21, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants