Remove TensorImpl::is_variable, deprecate Tensor::is_variable #29653

ezyang · 2019-11-12T16:30:13Z

Stack from ghstack:

Delete redefinitions of methods in Variable already present on Tensor. #29667 Delete redefinitions of methods in Variable already present on Tensor.
Move most methods off Variable into torch::autograd::impl functions. #29665 Move most methods off Variable into torch::autograd::impl functions.
Remove TensorImpl::is_variable, deprecate Tensor::is_variable #29653 Remove TensorImpl::is_variable, deprecate Tensor::is_variable

I didn't remove is_variable from Tensor for BC reasons, but I did
remove as many uses as I could from the codebase.
at::impl::variable_excluded_from_dispatch got moved to TensorBody.h
so that it's more widely accessible.

This diff is NOT semantics preserving. Here are the major differences:

In a number of native operator implementations, we tested that arguments
are not variable. I replaced these with asserts that variable is
excluded from dispatch. I actually don't think these asserts are really
necessary now (they should certainly be true, but it's hard to get
it wrong), but I've kept them for old time's sake. At least, they'll detect
if you call these functions before you've processed variable (indicating
a bug in your kernel.)
There are a number of places where we do a per-tensor test for being a
variable, for better error reporting when someone commits Tensor/Variable
confusion. Although these tests are substantively the same as the
tests above, in these cases I decided to delete the test entirely.
The reasoning is that in these cases, we didn't really care about
dispatch (also, see above; I'm not too sure we really need the dispatch
asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable
confusion is impossible now, we don't need the tests. One of the key
factors which pushed me one way or another was whether or not a function
was doing per-tensor validation; if I kept the assert in such functions,
I'd repeatedly access the TLS. Even if we want to bring back the asserts,
they would have to go somewhere else.

Another similar idiom is the number of places we do !x.defined() ||
x.is_variable(); I treated this equivalently.
nuclear_norm's computation of compute_uv is a bit weird, but I think
it's OK to just delete the is_variable case (I suspect that it is
always the case that self.is_variable(), but it doesn't really matter.)

Signed-off-by: Edward Z. Yang [email protected]

Differential Revision: D18496168

I didn't remove is_variable from Tensor for BC reasons, but I did remove as many uses as I could from the codebase. at::impl::variable_excluded_from_dispatch got moved to TensorBody.h so that it's more widely accessible. This diff is NOT semantics preserving. Here are the major differences: - In a number of native operator implementations, we tested that arguments are not variable. I replaced these with asserts that variable is excluded from dispatch. I actually don't think these asserts are really necessary now (they should certainly be true, but it's hard to get it wrong), but I've kept them for old time's sake. At least, they'll detect if you call these functions before you've processed variable (indicating a bug in your kernel.) - There are a number of places where we do a per-tensor test for being a variable, for better error reporting when someone commits Tensor/Variable confusion. Although these tests are substantively the same as the tests above, in these cases I decided to *delete* the test entirely. The reasoning is that in these cases, we didn't really care about dispatch (also, see above; I'm not too sure we really need the dispatch asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable confusion is impossible now, we don't need the tests. One of the key factors which pushed me one way or another was whether or not a function was doing per-tensor validation; if I kept the assert in such functions, I'd repeatedly access the TLS. Even if we want to bring back the asserts, they would have to go somewhere else. Another similar idiom is the number of places we do !x.defined() || x.is_variable(); I treated this equivalently. - nuclear_norm's computation of compute_uv is a bit weird, but I think it's OK to just delete the is_variable case (I *suspect* that it is always the case that self.is_variable(), but it doesn't really matter.) Signed-off-by: Edward Z. Yang <[email protected]> [ghstack-poisoned]

…ble" I didn't remove is_variable from Tensor for BC reasons, but I did remove as many uses as I could from the codebase. at::impl::variable_excluded_from_dispatch got moved to TensorBody.h so that it's more widely accessible. This diff is NOT semantics preserving. Here are the major differences: - In a number of native operator implementations, we tested that arguments are not variable. I replaced these with asserts that variable is excluded from dispatch. I actually don't think these asserts are really necessary now (they should certainly be true, but it's hard to get it wrong), but I've kept them for old time's sake. At least, they'll detect if you call these functions before you've processed variable (indicating a bug in your kernel.) - There are a number of places where we do a per-tensor test for being a variable, for better error reporting when someone commits Tensor/Variable confusion. Although these tests are substantively the same as the tests above, in these cases I decided to *delete* the test entirely. The reasoning is that in these cases, we didn't really care about dispatch (also, see above; I'm not too sure we really need the dispatch asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable confusion is impossible now, we don't need the tests. One of the key factors which pushed me one way or another was whether or not a function was doing per-tensor validation; if I kept the assert in such functions, I'd repeatedly access the TLS. Even if we want to bring back the asserts, they would have to go somewhere else. Another similar idiom is the number of places we do !x.defined() || x.is_variable(); I treated this equivalently. - nuclear_norm's computation of compute_uv is a bit weird, but I think it's OK to just delete the is_variable case (I *suspect* that it is always the case that self.is_variable(), but it doesn't really matter.) Signed-off-by: Edward Z. Yang <[email protected]> [ghstack-poisoned]

yf225 · 2019-11-12T17:36:52Z

aten/src/ATen/Utils.h

  }
-  if (expr.is_variable()) {  // TODO: change this to check `.requires_grad()` and `GradMode::is_enabled()` when Variable and Tensor are merged
-    AT_ERROR("Expected Tensor (not Variable) for argument #", pos, " '", name, "' in call to ", api);
-  }


Is it correct that we will never enter TH when VariableTensorId is still in the tensor type set? I feel that if the dispatch mechanism gives us this guarantee, then we can remove the check here, otherwise checking at::impl::variable_excluded_from_dispatch() might be easier to reason about.

TH legacy binding code always registers at a key like CPU or CUDA. You can't get there without handling Variable first. So there is indeed an invariant here; the error here is to guard against programmer error.

yf225 · 2019-11-12T17:45:15Z

torch/csrc/autograd/VariableTypeManual.cpp

-    if (!t.is_variable()) {
-      AT_ERROR("Expected object of type Variable but found type ", t.type().toString(), " at position #", i, " "
-                    "for iterable argument #", pos, " '", name, "'");
-    }


I believe it's okay to remove the checks in this file, based on the assumption that we never remove the VariableTensorId from the tensor type set prior to entering variable type dispatch (and maybe we already have a test for this assumption elsewhere?)

Yes, I agree with this claim. We don't have a global assert for this though.

kostmo · 2019-11-12T19:54:23Z

CircleCI build failures summary

As of commit 3be6674:

1/1 broken upstream at merge base 6d54c5d (see grid view)
- You may want to rebase on the viable/strict branch (see age history):
```
git fetch viable/strict
git rebase viable/strict
```
0/1 failures introduced in this PR
0/1 recognized as flaky

Here are the reasons each build failed:

Job	Step	Log excerpt
pytorch_xla_linux_xenial_py3_6_clang7_build	Build	`Failed to run '['/var/lib/jenkins/workspace/xla/scripts/generate_code.sh']'`

This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 2 time(s).

…ble" I didn't remove is_variable from Tensor for BC reasons, but I did remove as many uses as I could from the codebase. at::impl::variable_excluded_from_dispatch got moved to TensorBody.h so that it's more widely accessible. This diff is NOT semantics preserving. Here are the major differences: - In a number of native operator implementations, we tested that arguments are not variable. I replaced these with asserts that variable is excluded from dispatch. I actually don't think these asserts are really necessary now (they should certainly be true, but it's hard to get it wrong), but I've kept them for old time's sake. At least, they'll detect if you call these functions before you've processed variable (indicating a bug in your kernel.) - There are a number of places where we do a per-tensor test for being a variable, for better error reporting when someone commits Tensor/Variable confusion. Although these tests are substantively the same as the tests above, in these cases I decided to *delete* the test entirely. The reasoning is that in these cases, we didn't really care about dispatch (also, see above; I'm not too sure we really need the dispatch asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable confusion is impossible now, we don't need the tests. One of the key factors which pushed me one way or another was whether or not a function was doing per-tensor validation; if I kept the assert in such functions, I'd repeatedly access the TLS. Even if we want to bring back the asserts, they would have to go somewhere else. Another similar idiom is the number of places we do !x.defined() || x.is_variable(); I treated this equivalently. - nuclear_norm's computation of compute_uv is a bit weird, but I think it's OK to just delete the is_variable case (I *suspect* that it is always the case that self.is_variable(), but it doesn't really matter.) Signed-off-by: Edward Z. Yang <[email protected]> [ghstack-poisoned]

…ble" I didn't remove is_variable from Tensor for BC reasons, but I did remove as many uses as I could from the codebase. at::impl::variable_excluded_from_dispatch got moved to TensorBody.h so that it's more widely accessible. This diff is NOT semantics preserving. Here are the major differences: - In a number of native operator implementations, we tested that arguments are not variable. I replaced these with asserts that variable is excluded from dispatch. I actually don't think these asserts are really necessary now (they should certainly be true, but it's hard to get it wrong), but I've kept them for old time's sake. At least, they'll detect if you call these functions before you've processed variable (indicating a bug in your kernel.) - There are a number of places where we do a per-tensor test for being a variable, for better error reporting when someone commits Tensor/Variable confusion. Although these tests are substantively the same as the tests above, in these cases I decided to *delete* the test entirely. The reasoning is that in these cases, we didn't really care about dispatch (also, see above; I'm not too sure we really need the dispatch asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable confusion is impossible now, we don't need the tests. One of the key factors which pushed me one way or another was whether or not a function was doing per-tensor validation; if I kept the assert in such functions, I'd repeatedly access the TLS. Even if we want to bring back the asserts, they would have to go somewhere else. Another similar idiom is the number of places we do !x.defined() || x.is_variable(); I treated this equivalently. - nuclear_norm's computation of compute_uv is a bit weird, but I think it's OK to just delete the is_variable case (I *suspect* that it is always the case that self.is_variable(), but it doesn't really matter.) Signed-off-by: Edward Z. Yang <[email protected]> Differential Revision: [D18496168](https://our.internmc.facebook.com/intern/diff/D18496168) [ghstack-poisoned]

facebook-github-bot · 2019-11-14T21:43:11Z

@ezyang merged this pull request in 65bb34d.

Summary: Pull Request resolved: pytorch/pytorch#29653 I didn't remove is_variable from Tensor for BC reasons, but I did remove as many uses as I could from the codebase. at::impl::variable_excluded_from_dispatch got moved to TensorBody.h so that it's more widely accessible. This diff is NOT semantics preserving. Here are the major differences: - In a number of native operator implementations, we tested that arguments are not variable. I replaced these with asserts that variable is excluded from dispatch. I actually don't think these asserts are really necessary now (they should certainly be true, but it's hard to get it wrong), but I've kept them for old time's sake. At least, they'll detect if you call these functions before you've processed variable (indicating a bug in your kernel.) - There are a number of places where we do a per-tensor test for being a variable, for better error reporting when someone commits Tensor/Variable confusion. Although these tests are substantively the same as the tests above, in these cases I decided to *delete* the test entirely. The reasoning is that in these cases, we didn't really care about dispatch (also, see above; I'm not too sure we really need the dispatch asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable confusion is impossible now, we don't need the tests. One of the key factors which pushed me one way or another was whether or not a function was doing per-tensor validation; if I kept the assert in such functions, I'd repeatedly access the TLS. Even if we want to bring back the asserts, they would have to go somewhere else. Another similar idiom is the number of places we do !x.defined() || x.is_variable(); I treated this equivalently. - nuclear_norm's computation of compute_uv is a bit weird, but I think it's OK to just delete the is_variable case (I *suspect* that it is always the case that self.is_variable(), but it doesn't really matter.) Signed-off-by: Edward Z. Yang <[email protected]> Test Plan: Imported from OSS Differential Revision: D18496168 Pulled By: ezyang fbshipit-source-id: 5a1ded931e0c10a6b758ba64a8380d34110e0c3e

albanD · 2019-11-14T22:21:06Z

aten/src/ATen/core/function_schema.h

        alias_info_(std::move(alias_info)),
        is_inferred_type_(is_inferred_type) {
    if (default_value_ && default_value_->isTensor()) {
      auto t = default_value_->toTensor();


@ezyang what's the point of keeping the if statement if the assert does not exists? Is there some side effect that is not commented here?

You're right, there's no point.

…h#29653) Summary: Pull Request resolved: pytorch#29653 I didn't remove is_variable from Tensor for BC reasons, but I did remove as many uses as I could from the codebase. at::impl::variable_excluded_from_dispatch got moved to TensorBody.h so that it's more widely accessible. This diff is NOT semantics preserving. Here are the major differences: - In a number of native operator implementations, we tested that arguments are not variable. I replaced these with asserts that variable is excluded from dispatch. I actually don't think these asserts are really necessary now (they should certainly be true, but it's hard to get it wrong), but I've kept them for old time's sake. At least, they'll detect if you call these functions before you've processed variable (indicating a bug in your kernel.) - There are a number of places where we do a per-tensor test for being a variable, for better error reporting when someone commits Tensor/Variable confusion. Although these tests are substantively the same as the tests above, in these cases I decided to *delete* the test entirely. The reasoning is that in these cases, we didn't really care about dispatch (also, see above; I'm not too sure we really need the dispatch asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable confusion is impossible now, we don't need the tests. One of the key factors which pushed me one way or another was whether or not a function was doing per-tensor validation; if I kept the assert in such functions, I'd repeatedly access the TLS. Even if we want to bring back the asserts, they would have to go somewhere else. Another similar idiom is the number of places we do !x.defined() || x.is_variable(); I treated this equivalently. - nuclear_norm's computation of compute_uv is a bit weird, but I think it's OK to just delete the is_variable case (I *suspect* that it is always the case that self.is_variable(), but it doesn't really matter.) Signed-off-by: Edward Z. Yang <[email protected]> Test Plan: Imported from OSS Differential Revision: D18496168 Pulled By: ezyang fbshipit-source-id: 5a1ded931e0c10a6b758ba64a8380d34110e0c3e

I didn't remove is_variable from Tensor for BC reasons, but I did remove as many uses as I could from the codebase. at::impl::variable_excluded_from_dispatch got moved to TensorBody.h so that it's more widely accessible. This diff is NOT semantics preserving. Here are the major differences: - In a number of native operator implementations, we tested that arguments are not variable. I replaced these with asserts that variable is excluded from dispatch. I actually don't think these asserts are really necessary now (they should certainly be true, but it's hard to get it wrong), but I've kept them for old time's sake. At least, they'll detect if you call these functions before you've processed variable (indicating a bug in your kernel.) - There are a number of places where we do a per-tensor test for being a variable, for better error reporting when someone commits Tensor/Variable confusion. Although these tests are substantively the same as the tests above, in these cases I decided to *delete* the test entirely. The reasoning is that in these cases, we didn't really care about dispatch (also, see above; I'm not too sure we really need the dispatch asserts), we cared about Tensor/Variable confusion. Since Tensor/Variable confusion is impossible now, we don't need the tests. One of the key factors which pushed me one way or another was whether or not a function was doing per-tensor validation; if I kept the assert in such functions, I'd repeatedly access the TLS. Even if we want to bring back the asserts, they would have to go somewhere else. Another similar idiom is the number of places we do !x.defined() || x.is_variable(); I treated this equivalently. - nuclear_norm's computation of compute_uv is a bit weird, but I think it's OK to just delete the is_variable case (I *suspect* that it is always the case that self.is_variable(), but it doesn't really matter.) Signed-off-by: Edward Z. Yang <[email protected]> ghstack-source-id: 1ac0dea Pull Request resolved: pytorch#29653

…92168) When tracing a model using dynamo, theses assertions fail. Following #29653 and #46371, we think it might be OK to remove these two assertions as well. Pull Request resolved: #92168 Approved by: https://github.com/ezyang

ezyang requested a review from apaszke as a code owner November 12, 2019 16:30

This was referenced Nov 12, 2019

Rename getNonVariableDeprecatedTypeProperties to getDeprecatedTypeProperties #29203

Closed

Miscellaneous follow up for code review comments #29204

Closed

Delete all trivial uses of make_variable. #29213

Closed

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Nov 12, 2019

ezyang requested a review from yf225 November 12, 2019 16:41

yf225 reviewed Nov 12, 2019

View reviewed changes

yf225 approved these changes Nov 12, 2019

View reviewed changes

ezyang mentioned this pull request Nov 12, 2019

Move most methods off Variable into torch::autograd::impl functions. #29665

Closed

ezyang requested review from ebetica and goldsborough as code owners November 12, 2019 21:11

ezyang mentioned this pull request Nov 12, 2019

Delete redefinitions of methods in Variable already present on Tensor. #29667

Closed

ezyang added 2 commits November 13, 2019 12:44

facebook-github-bot closed this in 65bb34d Nov 14, 2019

ezyang mentioned this pull request Nov 14, 2019

Merge Tensor and Variable types. #28287

Closed

facebook-github-bot added the merged label Nov 14, 2019

albanD reviewed Nov 14, 2019

View reviewed changes

ezyang mentioned this pull request Nov 15, 2019

Add more defensive tests for undefined tensors. #29907

Closed

facebook-github-bot deleted the gh/ezyang/530/head branch November 18, 2019 15:16

gchanan added the module: deprecation label Jan 14, 2020

mruberry added the Merged label Oct 28, 2020

driazati mentioned this pull request Apr 20, 2021

Remove is_variable from tests #56305

Closed

ydwu4 mentioned this pull request Jan 13, 2023

Remove variable_excluded_from_dispatch() assertion from mkldnncommon #92168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove TensorImpl::is_variable, deprecate Tensor::is_variable #29653

Remove TensorImpl::is_variable, deprecate Tensor::is_variable #29653

Uh oh!

ezyang commented Nov 12, 2019 •

edited

Loading

Uh oh!

yf225 Nov 12, 2019

Uh oh!

ezyang Nov 12, 2019

Uh oh!

yf225 Nov 12, 2019

Uh oh!

ezyang Nov 12, 2019

Uh oh!

kostmo commented Nov 12, 2019 •

edited

Loading

Uh oh!

facebook-github-bot commented Nov 14, 2019

Uh oh!

albanD Nov 14, 2019

Uh oh!

ezyang Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Remove TensorImpl::is_variable, deprecate Tensor::is_variable #29653

Remove TensorImpl::is_variable, deprecate Tensor::is_variable #29653

Uh oh!

Conversation

ezyang commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

ezyang Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

ezyang Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

kostmo commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CircleCI build failures summary

Uh oh!

facebook-github-bot commented Nov 14, 2019

Uh oh!

albanD Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

ezyang Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ezyang commented Nov 12, 2019 •

edited

Loading

kostmo commented Nov 12, 2019 •

edited

Loading