Skip to content

Discover and check operator variants#76901

Closed
amjames wants to merge 9 commits intogh/amjames/3/basefrom
gh/amjames/3/head
Closed

Discover and check operator variants#76901
amjames wants to merge 9 commits intogh/amjames/3/basefrom
gh/amjames/3/head

Conversation

@amjames
Copy link
Copy Markdown
Collaborator

@amjames amjames commented May 5, 2022

Stack from ghstack (oldest at bottom):

Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
@amjames amjames requested review from mruberry and ngimel as code owners May 5, 2022 16:57
amjames added a commit that referenced this pull request May 5, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

ghstack-source-id: 67e7882
Pull Request resolved: #76901
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented May 5, 2022

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit 5f747a2 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Comment thread torch/testing/_internal/common_methods_invocations.py Outdated
Comment thread test/test_ops.py
Comment thread test/test_ops.py
Comment thread torch/testing/_internal/common_methods_invocations.py
Comment thread torch/testing/_internal/common_methods_invocations.py
@mruberry
Copy link
Copy Markdown
Collaborator

mruberry commented May 7, 2022

These PRs look great, @amjames! I triggered some additional tests (with the ciflow/all label) just in case. I also made one suggestion on comment readability -- which is not the fault of this PR, but while we're here I thought we might fix it, anyway, if you agree.

Let me know your thoughts and then once these tests pass I think this is good to land!

@lezcano
Copy link
Copy Markdown
Collaborator

lezcano commented May 8, 2022

Are there operations that we were testing manually before and now they are being collected automatically? If so, could you remove the OpInfos for those operations in this PR?

self.operator_variant = getattr(operator, self.name, None)

if self.inplace_operator_variant is _NOTHING:
if self.inplace_variant is not None:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@mruberry This is the logic I think may warrant additional explanation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sounds great -- it's hard to have too many comments

Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
amjames added a commit that referenced this pull request May 10, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

ghstack-source-id: fc00066
Pull Request resolved: #76901
@lezcano
Copy link
Copy Markdown
Collaborator

lezcano commented May 10, 2022

Doesn't this PR cover __rmatmul__? If so, what would be stopping us from adding the right-looking variants? It'd be so nice to get rid of __rmatmul__...

@amjames
Copy link
Copy Markdown
Collaborator Author

amjames commented May 10, 2022

@lezcano I could not find any OpInfos that are redundant due to the new operator capture rules. The only operators for which OpInfos are defined right now are the __r<op>__ variants. This PR does add OpInfo coverage for many __i<op>__ and a few __<op>__ methods which were missing before.

I did not add these reverse dunders to be auto-captured as they require additional kwargs to ensure they are not called with a Tensor on the LHS. There is currently an open issue (#74616) regarding problems coming from tests where reverse dunders are invoked manually with a Tensor on the lhs.

@mruberry
Copy link
Copy Markdown
Collaborator

@lezcano I could not find any OpInfos that are redundant due to the new operator capture rules. The only operators for which OpInfos are defined right now are the __r<op>__ variants. This PR does add OpInfo coverage for many __i<op>__ and a few __<op>__ methods which were missing before.

I did not add these reverse dunders to be auto-captured as they require additional kwargs to ensure they are not called with a Tensor on the LHS. There is currently an open issue (#74616) regarding problems coming from tests where reverse dunders are invoked manually with a Tensor on the lhs.

Instead of additional kwargs we might just want to create a sample inputs function that's just for the reverse dunders.

@mruberry
Copy link
Copy Markdown
Collaborator

Just ping me when this is ready for the next review, @amjames

@lezcano
Copy link
Copy Markdown
Collaborator

lezcano commented May 11, 2022

On the r-looking variants: My concern is that, at the moment, we are writing OpInfos for them manually and that's not great. For example, for non-trivial ones, like the one for matmul, the OpInfos for the normal operation and the r-looking one have diverged, and it's highly non-trivial to figure out whether they are equivalent or which one is correct, or whether both are missing things.

But again, perhaps merging the normal and r-looking operations is easier said than done, but yeah.

Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
amjames added a commit that referenced this pull request May 11, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

ghstack-source-id: a18df21
Pull Request resolved: #76901
@amjames
Copy link
Copy Markdown
Collaborator Author

amjames commented May 11, 2022

@mruberry This is ready for the next review. I am going to explore the idea of adding r-dunders re @lezcano 's ideas and will add a PR to the stack if that bears any fruit. CI failures appear to be infra related so hopefully they will all be green this time around.

Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
amjames added a commit that referenced this pull request May 11, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

ghstack-source-id: c0a74e2
Pull Request resolved: #76901
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
@mruberry
Copy link
Copy Markdown
Collaborator

Exploring more on the reverse dunders sounds cool but if the simplest solution is to keep them separate and give them their own sample inputs function that also sounds totally fine. Trying to have two sample inputs for already important and complicated operations like matmul might be too unreadable to be worthwhile.

@lezcano
Copy link
Copy Markdown
Collaborator

lezcano commented May 12, 2022

The inputs to rmatmul are those from matmul reversed. That's what led me to suggest automating all this really.

BinaryUfuncInfo('bitwise_or',
ref=np.bitwise_or,
dtypes=integral_types_and(torch.bool),
operator_variant=operator.or_,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice attention to the bitwise operators

Copy link
Copy Markdown
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

LGTM! We should wait for the tests to finish, but if they're OK you can merge this using @pytorchbot merge this

Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
amjames added a commit that referenced this pull request May 12, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

ghstack-source-id: 5af2ac5
Pull Request resolved: #76901
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
amjames added a commit that referenced this pull request May 13, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

ghstack-source-id: 92cdba7
Pull Request resolved: #76901
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
amjames added a commit that referenced this pull request May 13, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

ghstack-source-id: b04f19c
Pull Request resolved: #76901
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

[ghstack-poisoned]
amjames added a commit that referenced this pull request May 14, 2022
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

ghstack-source-id: 6a73a9f
Pull Request resolved: #76901
@amjames
Copy link
Copy Markdown
Collaborator Author

amjames commented May 15, 2022

@pytorchbot merge this

@github-actions
Copy link
Copy Markdown
Contributor

Hey @amjames.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request May 17, 2022
Summary:
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

Pull Request resolved: #76901

Approved by: https://github.com/mruberry

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/e5a752a6ca081045c25cd52afa9d245ed6582821

Reviewed By: atalman

Differential Revision: D36412522

Pulled By: atalman

fbshipit-source-id: cd8ca62d1e77f87b8d8d121f5d33dcffe82bf80d
@facebook-github-bot facebook-github-bot deleted the gh/amjames/3/head branch May 19, 2022 14:16
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 25, 2026
Operator variants can now be explicitly specified in the OpInfo kwargs.
When the operator name is not the same as the method/function form this
will allow them to be discovered.

The OpInfo is extended to also accept/discover the inplace operator
variant.

Operator and inplace operator variants are exercised in consistency
tests when the sample does not contain any kwargs.

Operations which require explicit declarations of operator and inplace
operator variants have had them added to their OpInfos.

Pull Request resolved: pytorch#76901

Approved by: https://github.com/mruberry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants