Skip to content

Refactor foreach unary ops tests to use OpInfo#49712

Closed
izdeby wants to merge 31 commits intogh/izdeby/73/basefrom
gh/izdeby/73/head
Closed

Refactor foreach unary ops tests to use OpInfo#49712
izdeby wants to merge 31 commits intogh/izdeby/73/basefrom
gh/izdeby/73/head

Conversation

@izdeby
Copy link
Copy Markdown
Contributor

@izdeby izdeby commented Dec 21, 2020

Stack from ghstack:

Differential Revision: D25673712


  • Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Dec 21, 2020

💊 CI failures summary and remediations

As of commit 69bfabb (more details on the Dr. CI page):


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-scanned failure(s)

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.

@izdeby izdeby mentioned this pull request Dec 21, 2020
@izdeby izdeby mentioned this pull request Jan 26, 2021
@izdeby izdeby changed the title Refactor Unary Ops tests Refactor foreach unary ops tests to use OpInfo Jan 26, 2021
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Iurii Zdebskyi added 2 commits January 27, 2021 09:23
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Iurii Zdebskyi and others added 8 commits March 2, 2021 13:46
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Copy link
Copy Markdown
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

Some questions but this looks like a reasonable refactor

check_foreach_api_restrictions(tensors);
TORCH_CHECK(tensors[0].scalar_type() != kBool,
"Negation, the `-` operator, on a bool tensor is not supported. "
"If you are trying to invert a mask, use the `~` or `logical_not()` operator instead.");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does ~ or logical_not work on TensorList right now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No.

import torch

a = [torch.rand(2, 2, device="cuda") for _ in range(2)]

print(torch.logical_not(a[0])) # fine
print(torch.logical_not(a)) # TypeError: logical_not(): argument 'input' (position 1) must be Tensor, not list
print(~a[0]) # TypeError: ~ (operator.invert) is only implemented on integer and Boolean-type tensors


a = [torch.rand(2, 2, device="cuda").to(torch.bool) for _ in range(2)]
print(torch.logical_not(a[0])) # fine
print(~a[0]) # fine

print(torch.logical_not(a)) # TypeError: logical_not(): argument 'input' (position 1) must be Tensor, not list
print(~a) # TypeError: bad operand type for unary ~: 'list'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we improve the error message? Some things I am noticing:

  1. If the user gets this error message, they did not actually use the - builtin operator. E.g. a user didn't do tensors = [...]; other_tensors = -tensors. So the "the - operator" part of the error message is confusing
  2. The error message assumes that the user applied operations to a bool tensor. Maybe we should mention that the API being called is "_foreach_neg"
  3. The error message shows up after a user calls _foreach_neg. But the error message suggests workarounds for a single tensor. This may be misunderstood to "just use ~ on your tensorlist"

Adding all the top together, maybe something like the following would make more sense?

_foreach_neg: There is a bool tensor in the passed-in TensorList.
Negation on a bool tensor is not supported. If you are trying to invert a mask, please
use the `~` or `logical_not()` operator on the individual tensors instead

Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
@izdeby izdeby requested a review from zou3519 March 16, 2021 17:21
Copy link
Copy Markdown
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

The testing refactor LGTM. @mruberry do you have other comments about how OpInfo is being used?

safe_casts_outputs=False),

ForeachUnaryFuncInfo('_foreach_sqrt',
method=torch._foreach_sqrt,
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.

Definitely not a blocker for this PR, but one thing we do with the base OpInfos is attempt to programmatically acquire the method and inplace variants of an op, which this could do, too, if they weren't specified.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

dtypesIfCPU=floating_and_complex_types_and(torch.bfloat16),
dtypesIfCUDA=floating_and_complex_types_and(torch.half)),

ForeachUnaryFuncInfo('_foreach_exp',
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.

These names could probably also be auto-generated by default from a string like 'exp', and the reference op also acquired from the same string automatically.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@mruberry
Copy link
Copy Markdown
Collaborator

The testing refactor LGTM. @mruberry do you have other comments about how OpInfo is being used?

This is a smart use of the OpInfo pattern. I made a couple suggestions for ideas that might be nice to have in the future, but this LGTM!

Copy link
Copy Markdown
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

approving, but please see nit on the error message

izdeby added 2 commits March 18, 2021 15:16
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
@izdeby izdeby changed the title Refactor foreach unary ops tests to use OpInfo [wip] Refactor foreach unary ops tests to use OpInfo Mar 18, 2021
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712)

-----
- Updated foreach unary ops tests to use OpInfo

[ghstack-poisoned]
@izdeby izdeby changed the title [wip] Refactor foreach unary ops tests to use OpInfo Refactor foreach unary ops tests to use OpInfo Mar 18, 2021
@izdeby izdeby requested review from mruberry and zou3519 March 18, 2021 23:39
@izdeby
Copy link
Copy Markdown
Contributor Author

izdeby commented Mar 18, 2021

approving, but please see nit on the error message

Done!

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@izdeby merged this pull request in cc7a28d.

@facebook-github-bot facebook-github-bot deleted the gh/izdeby/73/head branch March 23, 2021 14:16
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.

4 participants