Refactor foreach unary ops tests to use OpInfo#49712
Refactor foreach unary ops tests to use OpInfo#49712izdeby wants to merge 31 commits intogh/izdeby/73/basefrom
Conversation
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 69bfabb (more details on the Dr. CI page):
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. |
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712) [ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712) [ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712) [ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712) [ghstack-poisoned]
Differential Revision: [D25673712](https://our.internmc.facebook.com/intern/diff/D25673712) [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]
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]
zou3519
left a comment
There was a problem hiding this comment.
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."); |
There was a problem hiding this comment.
Does ~ or logical_not work on TensorList right now?
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
Should we improve the error message? Some things I am noticing:
- If the user gets this error message, they did not actually use the
-builtin operator. E.g. a user didn't dotensors = [...]; other_tensors = -tensors. So the "the-operator" part of the error message is confusing - 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"
- 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]
| safe_casts_outputs=False), | ||
|
|
||
| ForeachUnaryFuncInfo('_foreach_sqrt', | ||
| method=torch._foreach_sqrt, |
There was a problem hiding this comment.
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.
| dtypesIfCPU=floating_and_complex_types_and(torch.bfloat16), | ||
| dtypesIfCUDA=floating_and_complex_types_and(torch.half)), | ||
|
|
||
| ForeachUnaryFuncInfo('_foreach_exp', |
There was a problem hiding this comment.
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.
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! |
zou3519
left a comment
There was a problem hiding this comment.
approving, but please see nit on the error message
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]
Done! |
Stack from ghstack:
Differential Revision: D25673712