Fix some tensor operators to return `NotImplemented` for invalid inputs by asi1024 · Pull Request #57934 · pytorch/pytorch

asi1024 · 2021-05-10T10:39:11Z

This PR fixes torch.Tensor{__rsub__, __rdiv__, __rtruediv__, __pow__, __rmatmul__} to return NotImplemented instead of raising a TypeError.

cc/ @mruberry: The first commit of this PR is the same as 1d209db excepts the commit message.

facebook-github-bot · 2021-05-10T10:39:17Z

💊 CI failures summary and remediations

As of commit 3069f58 (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.

Click here to manually regenerate this comment.

albanD

Thanks for the PR that looks quite good.
I am not sure what test_binary_ufuncs is testing for these. In particular, does it properly cover all the reverse cases and make sure they return the right value?

albanD · 2021-05-10T13:48:00Z

  {"__nonzero__", THPVariable_bool_scalar, METH_NOARGS, NULL},
  {"__invert__", THPVariable_invert, METH_NOARGS, NULL},
  {"__matmul__", castPyCFunctionWithKeywords(TypeError_to_NotImplemented_<THPVariable_matmul>), METH_VARARGS | METH_KEYWORDS, NULL},
+  {"__rmatmul__", castPyCFunctionWithKeywords(TypeError_to_NotImplemented_<THPVariable_matmul>), METH_VARARGS | METH_KEYWORDS, NULL},


Is THPVariable_matmul actually handling the reverse version properly?
In particular if two valid inputs are given to it and it does not throw an error.

codecov · 2021-05-10T16:39:02Z

Codecov Report

Merging #57934 (3069f58) into master (259d19a) will increase coverage by 0.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master   #57934      +/-   ##
==========================================
+ Coverage   76.82%   76.84%   +0.01%     
==========================================
  Files        1986     1986              
  Lines      197448   197459      +11     
==========================================
+ Hits       151693   151735      +42     
+ Misses      45755    45724      -31

asi1024 · 2021-05-10T19:41:55Z

@albanD Thank you for your review!
I implemented __rmatmul__ method in _tensor.py to handle the reverse version properly, and also add a test! Could you take another look? 😃

asi1024 · 2021-05-10T19:54:36Z

BTW, when I created this PR, I had a misunderstanding that I need to fix __rmatmul__, which is already supported, to return NotImplemented. The support of __rmatmul__ is outside of the meaning of the title of this PR. Should I separate it into another PR?

albanD · 2021-05-10T23:36:59Z

Thanks for the update.
I think you can leave it here as it is not too big.

Could you make sure that the CI in green (in particular, flake is not happy with the assert False, you can replace it with raise RuntimeError...).
Also looks like your rmatmul implementation goes into an infinite recursion.

mruberry · 2021-05-11T05:22:20Z

I suggest just updating those that are easiest for the tests to pass and leaving a comment in the code about the remainders. This is a nice code cleanup but few users will appreciate the change

albanD

LGTM thanks for the tests

facebook-github-bot · 2021-05-11T13:47:31Z

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

mruberry · 2021-05-11T16:36:42Z

Thanks @asi1024!

asi1024 · 2021-05-11T17:13:57Z

Thanks! 😄

facebook-github-bot · 2021-05-12T18:04:29Z

@albanD merged this pull request in 35521a2.

albanD · 2021-05-12T20:57:18Z

I'm afraid this broke some master-only CI builds: https://app.circleci.com/pipelines/github/pytorch/pytorch/320932/workflows/2a61be9b-ecad-475b-bb9a-0c03b6c97d71/jobs/13402438

I think bfloat16 should be added to the cuda types.

Could you send a new PR with the same content as this one + the bfloat16 dtype on cuda?

(the bot is going to mark this one reverted very soon)

facebook-github-bot · 2021-05-12T20:59:18Z

This pull request has been reverted by 5e83c62.

asi1024 · 2021-05-13T04:51:18Z

@albanD I reopened a PR in #58216!

…ts (pytorch#57934) Summary: Fixes pytorch#57719. This PR fixes `torch.Tensor{__rsub__, __rdiv__, __rtruediv__, __pow__, __rmatmul__}` to return `NotImplemented` instead of raising a `TypeError`. cc/ mruberry: The first commit of this PR is the same as pytorch@1d209db excepts the commit message. Pull Request resolved: pytorch#57934 Reviewed By: mruberry Differential Revision: D28351931 Pulled By: albanD fbshipit-source-id: 985457a44dba24d2496794dfb8c1661cbcd4ff8f

…ts (#58216) Summary: Same as #57934. (cc/ albanD) Pull Request resolved: #58216 Reviewed By: ailzhang Differential Revision: D28494886 Pulled By: albanD fbshipit-source-id: 380205867ee1cde90e1c6fcfe2a31749e1243530

…ts (pytorch#57934) Summary: Fixes pytorch#57719. This PR fixes `torch.Tensor{__rsub__, __rdiv__, __rtruediv__, __pow__, __rmatmul__}` to return `NotImplemented` instead of raising a `TypeError`. cc/ mruberry: The first commit of this PR is the same as pytorch@1d209db excepts the commit message. Pull Request resolved: pytorch#57934 Reviewed By: mruberry Differential Revision: D28351931 Pulled By: albanD fbshipit-source-id: 985457a44dba24d2496794dfb8c1661cbcd4ff8f

…ts (pytorch#58216) Summary: Same as pytorch#57934. (cc/ albanD) Pull Request resolved: pytorch#58216 Reviewed By: ailzhang Differential Revision: D28494886 Pulled By: albanD fbshipit-source-id: 380205867ee1cde90e1c6fcfe2a31749e1243530

asi1024 added 2 commits May 10, 2021 10:27

Fix some operators to return NotImplemented for invalid inputs

3650d72

Fix variable name in test/onnx/expect/TestOperators.test_rsub.expect

1d219b1

asi1024 requested review from BowenBao, albanD, neginraoof, soulitzer and spandantiwari as code owners May 10, 2021 10:39

facebook-github-bot added the cla signed label May 10, 2021

pytorchbot added the open source label May 10, 2021

albanD reviewed May 10, 2021

View reviewed changes

bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 10, 2021

asi1024 force-pushed the fix-operator branch from 6fb42c4 to f81b5d6 Compare May 11, 2021 08:51

Add torch.__rmatmul__

3069f58

asi1024 force-pushed the fix-operator branch from f81b5d6 to 3069f58 Compare May 11, 2021 11:20

albanD approved these changes May 11, 2021

View reviewed changes

facebook-github-bot closed this in 35521a2 May 12, 2021

facebook-github-bot added the Merged label May 12, 2021

facebook-github-bot added the Reverted label May 12, 2021

asi1024 mentioned this pull request May 13, 2021

Fix some tensor operators to return NotImplemented for invalid inputs #58216

Closed

Conversation

asi1024 commented May 10, 2021

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD May 10, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

asi1024 commented May 10, 2021

Uh oh!

asi1024 commented May 10, 2021

Uh oh!

albanD commented May 10, 2021

Uh oh!

mruberry commented May 11, 2021

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 11, 2021

Uh oh!

mruberry commented May 11, 2021

Uh oh!

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

Uh oh!

Uh oh!

facebook-github-bot commented May 12, 2021

Uh oh!

albanD commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 12, 2021

Uh oh!

asi1024 commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

facebook-github-bot commented May 10, 2021 •

edited

Loading

codecov Bot commented May 10, 2021 •

edited

Loading

asi1024 commented May 11, 2021 •

edited

Loading

albanD commented May 12, 2021 •

edited

Loading