Introduced AliasInfo for OpInfo by vfdev-5 · Pull Request #50368 · pytorch/pytorch

vfdev-5 · 2021-01-11T10:54:50Z

Introduced AliasInfo for OpInfo.

Context: Split of #49158

cc @mruberry , please let me know if you'd like to see here more code to cover

[ ] fold test_op_aliases.py into OpInfo-based testing in test_ops.py

from #50006

and/or add UnaryUfuncInfo('abs') as discussed https://github.com/pytorch/pytorch/pull/49158/files#r548774221

Context: pytorch#49158

facebook-github-bot · 2021-01-11T10:55:02Z

💊 CI failures summary and remediations

As of commit 3d32fa4 (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 to the (internal) Dr. CI Users group.

codecov · 2021-01-11T14:14:33Z

Codecov Report

Merging #50368 (3d32fa4) into master (f7e90cf) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #50368      +/-   ##
==========================================
- Coverage   80.87%   80.72%   -0.16%     
==========================================
  Files        1931     1931              
  Lines      210690   210701      +11     
==========================================
- Hits       170406   170085     -321     
- Misses      40284    40616     +332

mruberry · 2021-01-13T10:14:29Z

Hey @vfdev-5, this is exactly what I was thinking. I think we should extend this PR a little further like you suggest.

Let's port

pytorch/test/test_op_aliases.py

Line 186 in 4a3a378

def _test_jit_op_alias_normalization(self, device, info=info):

and

pytorch/test/test_op_aliases.py

Line 248 in 4a3a378

def _test_alias_computation(self, device, info=info):

to test_ops.py and update to be run on OpInfos that have one or more AliasInfos. This is going to be a little tricky because you'll have to separate those tests from the custom test generator they're in. The original tests should remain for now, too, until we port all the aliases in test_op_aliases.py to OpInfo metadata. To test that these two new alias tests are working properly we can add an AliasInfo to acos:

pytorch/torch/testing/_internal/common_methods_invocations.py

Line 742 in 4a3a378

UnaryUfuncInfo('acos',

pytorch/test/test_op_aliases.py

Line 56 in 4a3a378

AliasInfo('arccos', torch.arccos, 'acos', torch.acos,

And remove the arccos entries from alias_infos.

Then after we get the new tests working and demonstrate that by updating an existing OpInfo to use the pattern we can follow-up by porting the other alias_infos and delete test_op_aliases.py. Sound good?

vfdev-5 · 2021-01-13T10:23:03Z

@mruberry thanks a lot for the review and a thourough description on the improvements ! Sounds great ! Let me update the PR and I'll let you know once it is ready for another review round.

…stCommon

…opinfo-aliases

mruberry

Hey @vfdev-5! This is really cool. All the OpInfo changes look great. I have two questions about the tests which I inline, but can be summarized as:

for the eager consistency test, should we just add alias variants as variants?
for the jit consistency test, should we just check that the jit correctly remaps the operation in jit graphs, instead, like the current alias tests do?

Looking forward to hearing your thoughts. This new approach to alias testing (having to just add one alias line in an OpInfo) is way better than the old approach.

Last issue is that this should update this guidance:

pytorch/aten/src/ATen/native/native_functions.yaml

Line 230 in 627a331

# 7) Add entries to test/test_op_aliases.py's "alias_infos"

to reflect the new approach. Basically I think we should tell people to add the line to an op's OpInfo if the OpInfo exists, and add the OpInfo (and then add the line) if it doesn't exist.

…opinfo-aliases

- jit_name_remapping instead jit consistency - removed auto generated tests for abs - added aliases tests inside test_variant_consistency_eager - added aliases tests to test_out

- added jit scripting in test_jit_alias_remapping - etc

mruberry · 2021-01-27T17:08:01Z

+            _test(a_op)
+
+    @ops([op for op in op_db if op.aliases])
+    def test_jit_alias_remapping(self, device, dtype, op):


@eellison would you take a look at this port of the test from test_op_aliases.py? @vfdev-5 is creating a version of that test that uses OpInfos instead of the custom AliasInfos that test_op_aliases.py uses. The idea is to eventually remove the AliasInfos by adding their metadata to OpInfos.

vfdev-5 · 2021-02-01T09:52:04Z

@eellison could you please review this PR in order to unblock other PRs. Thanks !

eellison

LGTM

mruberry

Cool! Thanks @vfdev-5! It's great to have the architecture to further simplify our test suite by making alias testing as simple as adding a line of metadata to an OpInfo.

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-02-02T09:12:51Z

@mruberry merged this pull request in b106250.

Summary: Description: - Added clamp, trunc tests with aliases - Added tests for aliases for asin(h), acos(h), etc - fixed 'fix' alias implementation - fixed annotations in test_jit_alias_remapping - updated native_functions.yaml aliases guidelines Blocked by #50368 cc mruberry Pull Request resolved: #51167 Reviewed By: gchanan Differential Revision: D26245753 Pulled By: mruberry fbshipit-source-id: e17b657f0515139735a8a677b1ae284904f98aef

Summary: Description: - Added clamp, trunc tests with aliases - Added tests for aliases for asin(h), acos(h), etc - fixed 'fix' alias implementation - fixed annotations in test_jit_alias_remapping - updated native_functions.yaml aliases guidelines Blocked by pytorch#50368 cc mruberry Pull Request resolved: pytorch#51167 Reviewed By: gchanan Differential Revision: D26245753 Pulled By: mruberry fbshipit-source-id: e17b657f0515139735a8a677b1ae284904f98aef

Summary: Introduced AliasInfo for OpInfo. Context: Split of pytorch#49158 cc mruberry , please let me know if you'd like to see here more code to cover > [ ] fold test_op_aliases.py into OpInfo-based testing in test_ops.py from pytorch#50006 and/or add `UnaryUfuncInfo('abs')` as discussed https://github.com/pytorch/pytorch/pull/49158/files#r548774221 Pull Request resolved: pytorch#50368 Reviewed By: ngimel Differential Revision: D26177261 Pulled By: mruberry fbshipit-source-id: 2e3884a387e8d5365fe05945375f0a9d1b5f5d82

Summary: Description: - Added clamp, trunc tests with aliases - Added tests for aliases for asin(h), acos(h), etc - fixed 'fix' alias implementation - fixed annotations in test_jit_alias_remapping - updated native_functions.yaml aliases guidelines Blocked by pytorch#50368 cc mruberry Pull Request resolved: pytorch#51167 Reviewed By: gchanan Differential Revision: D26245753 Pulled By: mruberry fbshipit-source-id: e17b657f0515139735a8a677b1ae284904f98aef

Introduced AliasInfo for OpInfo

c21bfe0

Context: pytorch#49158

facebook-github-bot added the cla signed label Jan 11, 2021

pytorchbot added the open source label Jan 11, 2021

zhangguanheng66 added module: testing Issues related to the torch.testing module (not tests) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Jan 11, 2021

zhangguanheng66 requested a review from mruberry January 11, 2021 16:19

Merge branch 'master' into add-opinfo-aliases

b6ce6bf

mruberry reviewed Jan 13, 2021

View reviewed changes

Comment thread torch/testing/_internal/common_methods_invocations.py Outdated

vfdev-5 added 2 commits January 13, 2021 08:00

Added arcsin alias, added abs UnaryUfuncInfo

e4cde50

[WIP] Added test_aliases_consistency_eager/jit methods to test_ops.Te…

65b39de

…stCommon

vfdev-5 commented Jan 13, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

vfdev-5 commented Jan 13, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

vfdev-5 added 2 commits January 19, 2021 11:11

Merge branch 'master' of https://github.com/pytorch/pytorch into add-…

b2917b5

…opinfo-aliases

Fixed test_aliases_consistency_jit

4134a45

vfdev-5 requested a review from mruberry January 20, 2021 00:51

vfdev-5 and others added 2 commits January 25, 2021 09:36

Merge branch 'master' into add-opinfo-aliases

b1c225c

Fixed failing tests

306420b

vfdev-5 force-pushed the add-opinfo-aliases branch from 6a835d1 to 306420b Compare January 25, 2021 10:35

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread torch/testing/_internal/common_methods_invocations.py Outdated

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread torch/testing/_internal/common_methods_invocations.py Outdated

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

mruberry reviewed Jan 25, 2021

View reviewed changes

Merge branch 'master' of https://github.com/pytorch/pytorch into add-…

159fc21

…opinfo-aliases

mruberry reviewed Jan 25, 2021

View reviewed changes

Comment thread torch/testing/_internal/common_methods_invocations.py

Updated code according to PR's review

00f7dfb

- jit_name_remapping instead jit consistency - removed auto generated tests for abs - added aliases tests inside test_variant_consistency_eager - added aliases tests to test_out

vfdev-5 commented Jan 25, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

Fixed typing issue by ignoring the type

e699695

mruberry reviewed Jan 26, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

mruberry reviewed Jan 26, 2021

View reviewed changes

Comment thread test/test_ops.py

mruberry reviewed Jan 26, 2021

View reviewed changes

Comment thread test/test_ops.py Outdated

mruberry reviewed Jan 26, 2021

View reviewed changes

Comment thread torch/testing/_internal/common_methods_invocations.py Outdated

Updates according to the review

38cab9f

- added jit scripting in test_jit_alias_remapping - etc

vfdev-5 force-pushed the add-opinfo-aliases branch from 8beb0c9 to 38cab9f Compare January 26, 2021 22:25

vfdev-5 mentioned this pull request Jan 27, 2021

OpInfo: Added clamp and trunc tests with aliases #51167

Closed

mruberry reviewed Jan 27, 2021

View reviewed changes

eellison self-requested a review January 27, 2021 17:44

vfdev-5 added 2 commits January 27, 2021 22:35

Merge branch 'master' into add-opinfo-aliases

38bed6c

Fixed flake8

3d32fa4

eellison approved these changes Feb 1, 2021

View reviewed changes

mruberry self-requested a review February 1, 2021 17:38

mruberry approved these changes Feb 1, 2021

View reviewed changes

facebook-github-bot reviewed Feb 1, 2021

View reviewed changes

facebook-github-bot closed this in b106250 Feb 2, 2021

vfdev-5 deleted the add-opinfo-aliases branch February 2, 2021 08:39

facebook-github-bot added the Merged label Feb 2, 2021

Conversation

vfdev-5 commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

codecov Bot commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

mruberry commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vfdev-5 commented Jan 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mruberry Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

vfdev-5 commented Feb 1, 2021

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vfdev-5 commented Jan 11, 2021 •

edited

Loading

facebook-github-bot commented Jan 11, 2021 •

edited

Loading

codecov Bot commented Jan 11, 2021 •

edited

Loading

mruberry commented Jan 13, 2021 •

edited

Loading