fix CompositeImplicitAutograd ops improperly labeled #69169
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit ceb7849 (more details on the Dr. CI page):
🕵️ 7 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
|
@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
zou3519
reviewed
Nov 30, 2021
8000
| @@ -922,10 +922,14 @@ | |||
| - func: logical_not(Tensor self) -> Tensor | |||
| device_check: NoCheck # TensorIterator | |||
| variants: function, method | |||
| dispatch: | |||
| CompositeExplicitAutograd: logical_not | |||
There was a problem hiding this comment.
You also need to add a backward formula that says the input is not differentiable, right?
e.g.
pytorch/tools/autograd/derivatives.yaml
Lines 2417 to 2419 in aa2163e
There was a problem hiding this comment.
I feel like we should do this by default, this is a common thing folks miss
There was a problem hiding this comment.
Do you mean to add this because we don't want to raise an error if the inputs require grad? Is it possible for it to not raise an error here even without the formula, because the output is bool... i.e, the number of differentiable outputs is 0.
Edit: or perhaps because it doesn't have a differentiable info at all, it would automatically skip to adding the torch checks without first checking the dtypes of the outputs.
There was a problem hiding this comment.
Without this PR, logical_not does not raise an error when passed inputs that require_grad:
>>> import torch
>>> x = torch.randn(3, requires_grad=True)
>>> torch.logical_not(x)
tensor([False, False, False])
So we probably want to replicate that behavior, and to replicate that behavior the way I know of is to add the backward formula (otherwise, the operator errors out, right?)
There was a problem hiding this comment.
Ah yeah you are definitely right here. Codegen has no way of checking the dtype of the output, so it can only rely on output_differentiability, which can only exist if the formula exists. More generally though the logic is slightly more involved.
For example new_zeros is composite, but doesn't have a formula, and shouldn't error because it is listed in DONT_REQUIRE_DERIVATIVE.
There was a problem hiding this comment.
Yes, we want to make sure that these do no fail when passed an input that requires gradient. Only the output should not have requires grad.
zou3519
reviewed
Nov 30, 2021
Comment on lines
+982
to
+983
| dispatch: | ||
| CompositeExplicitAutograd: logical_or_ |
There was a problem hiding this comment.
(I think) This PR fixes the root cause of the composite compliance test failure for these operators (
pytorch/torch/testing/_internal/common_methods_invocations.py
Lines 9707 to 9708 in aa2163e
I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) [ghstack-poisoned]
|
@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
bdhirsh
commented
Dec 1, 2021
tools/autograd/derivatives.yaml
Outdated
| output_differentiability: [False] | ||
|
|
||
| - name: logical_and(Tensor self, Tensor other) -> Tensor | ||
| output_differentiability: [False] |
There was a problem hiding this comment.
@soulitzer what's the best way to declare these ops as non-differentiable? I guess the best case scenario would be if we can error out immediately in the forward pass, if someone tries to call torch.logical_and(torch.ones(2, requres_grad=True)).
I'm getting codegen errors that say RuntimeError: output_differentiability=False for inplace operation (version_counter won't get updated). I see another option from the docs is to add these ops to the DONT_REQUIRE_DERIVATIVE in gen_variable_type?
There was a problem hiding this comment.
It is pretty counterintuitive but declaring these ops as non-differentiable actually makes it so that they do not error when someone passes in a tensor that has requires_grad=True. The default behavior when no formula is specified is actually to error immediately in the forward pass.
A good example of why that is the case is eq, we wouldn't want users to have to detach everytime they want to compare two tensors. Maybe it is less clear what we really want to do in the logical_{and,or} cases, but adding it would be good to avoid bc-breaking issues.
I'm actually wondering how outdated that error is, since we update the version counter in the ADInplaceOrView kernel now. Perhaps we should just remove it?
It is also weird that eq does a similar thing (i.e., it has a in-place variant), but doesn't run into the error. I'm not sure why eq is able to work, but if you look into that perhaps you can emulate how it is done there.
Adding to DONT_REQUIRE_DERIVATIVE should also work, so I guess we can do that as a easy workaround for now.
There was a problem hiding this comment.
A good example of why that is the case is eq, we wouldn't want users to have to detach everytime they want to compare two tensors.
Ah okay, then keeping the current behavior of not immediately erroring out in the forward pass sounds reasonable, to not be BC-breaking.
Adding to DONT_REQUIRE_DERIVATIVE should also work, so I guess we can do that as a easy workaround for now.
I'll try this for now, th
9E88
anks! I looked at eq and I don't think I'm doing anything differently for logical_and/xor/not.
I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) Differential Revision: [D32739976](https://our.internmc.facebook.com/intern/diff/D32739976) [ghstack-poisoned]
|
@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) Differential Revision: [D32739976](https://our.internmc.facebook.com/intern/diff/D32739976) [ghstack-poisoned]
|
@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) Differential Revision: [D32739976](https://our.internmc.facebook.com/intern/diff/D32739976) [ghstack-poisoned]
|
@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@soulitzer @zou3519 I think this is ready for another round of review |
I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) Differential Revision: [D32739976](https://our.internmc.facebook.com/intern/diff/D32739976) [ghstack-poisoned]
|
@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
soulitzer
approved these changes
Dec 9, 2021
There was a problem hiding this comment.
LGTM
… for a composite op with no derivative formula" Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator ``` [ghstack-poisoned]
bdhirsh
added a commit
that referenced
this pull request
Dec 10, 2021
… op with no derivative formula" Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator ``` [ghstack-poisoned]
PaliC
added a commit
that referenced
this pull request
Dec 10, 2021
Summary: Pull Request resolved: #69169 I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) Test Plan: Imported from OSS Reviewed By: zou3519 Differential Revision: D32739976 Pulled By: bdhirsh fbshipit-source-id: a756dd9e0b87276368063c8f4934be59dca371d3
desertfire
pushed a commit
that referenced
this pull request
Dec 13, 2021
Summary: Pull Request resolved: #69169 I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) Test Plan: Imported from OSS Reviewed By: zou3519 Differential Revision: D32739976 Pulled By: bdhirsh fbshipit-source-id: a756dd9e0b87276368063c8f4934be59dca371d3
desertfire
pushed a commit
that referenced
this pull request
Dec 14, 2021
Summary: Pull Request resolved: #69169 I checked `derivatives.yaml`, and it doesn't look like `logical_not/and/xor` are meant to work with autograd. Those 3 ops are currently set as `CompositeImplicitAutograd` though, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead. This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090) Test Plan: Imported from OSS Reviewed By: zou3519 Differential Revision: D32739976 Pulled By: bdhirsh fbshipit-source-id: a756dd9e0b87276368063c8f4934be59dca371d3
bdhirsh
added a commit
that referenced
this pull request
Dec 14, 2021
… for a composite op with no derivative formula" Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator ``` Differential Revision: [D33018607](https://our.internmc.facebook.com/intern/diff/D33018607) [ghstack-poisoned]
bdhirsh
added a commit
that referenced
this pull request
Dec 14, 2021
… op with no derivative formula" Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator Error if someone tries to modify the yaml by adding a composite op that would result in losing autograd for that op (because we can no longer use the composite formula). This fails locally (correctly) on master because `logical_and` is improperly labeled as `CompositeExplicitAutograd` but doesn't have a derivative formula. So I'll merge this after #69169 goes through. Failure error: ``` AssertionError: Found an entry for 'logical_not' in the yaml. This operator is composite, which means that by default it will decompose into base operators. If decomposing is fine, then you can simply remove the entry from the yaml file. If you wish to provide an explicit kernel directly for this composite op, you can do so by: (a) moving the operator name under the 'autograd' entry in the yaml file (b) writing a custom Autograd Function for the operator ``` Differential Revision: [D33018607](https://our.internmc.facebook.com/intern/diff/D33018607) [ghstack-poisoned]
|
This pull request has been reverted by 41c344d. To re-land this change, follow these steps. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I checked
derivatives.yaml, and it doesn't look likelogical_not/and/xorare meant to work with autograd. Those 3 ops are currently set asCompositeImplicitAutogradthough, implying that they do work with autograd. Updating them to be CompositeExplicitAutograd instead.This came up because I'm trying to improve the error checking in external backend codegen, and these ops being improperly labeled incorrectly triggers my new error checks for XLA (see #67090)
Stack from ghstack:
Differential Revision: D32739976