Move VariableImpl functions to AutogradMeta and Variable #15487

yf225 · 2018-12-21T19:45:51Z

In this PR, we are moving all functions away from Variable::Impl, in order to get rid of Variable::Impl (and the data_ Tensor in it) in the next PR. Some of the functions (such as set_requires_grad / requires_grad / grad) will be living in AutogradMeta class, while others (such as backward() / rebase_history() / grad_accumulator() / grad_fn()) will be living in Variable class.

This is the 2nd PR mentioned in #13638.

yf225 · 2018-12-21T20:23:30Z

c10/core/TensorImpl.h

+  virtual void set_requires_grad(bool requires_grad, at::TensorImpl* self_impl) = 0;
+  virtual bool requires_grad() const = 0;
+  virtual at::Tensor& grad() = 0;
+  virtual const at::Tensor& grad() const = 0;


We add these functions into AutogradMetaInterface, and call them from TensorImpl's corresponding functions, because Variable::Impl will not be overriding those functions anymore (and Variable::Impl will be removed in the next PR).

torch/csrc/autograd/variable.cpp

ezyang · 2018-12-21T20:53:07Z

c10/core/TensorImpl.cpp

+  if (autograd_meta()) {
+    return autograd_meta()->grad();
+  } else {
+    AT_ERROR("grad is not implemented for Tensor");


This error message is not too accurate now, right?

I feel that it's still correct, because these functions still only work on Variable and not Tensor. A more detailed version might be Cannot call grad on Tensor that doesn't contain autograd metadata, but only Variable contains autograd metadata right now, so the original error message is probably ok

it sounds like it's correct until we actually merge variable and tensor. It's worth thinking about if this error message will naturally go away in that case, or you'll have to do active work to make it go away. In either case, a FixMe should be here; in the later case, it's probably worth filing an issue so you don't forget.

I feel that it might be worthwhile to get rid of set_requires_grad / requires_grad / grad in TensorImpl and move them to Variable class, since people can only call those functions on a Variable anyway.

EDIT: This line https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/core/jit_type.h#L360 accesses tensor.requires_grad() from ATen, so we likely still need to keep set_requires_grad / requires_grad / grad in Tensor/TensorImpl instead of moving them to Variable.

gchanan · 2018-12-21T23:40:54Z

I don't quite get the argument:

Variable would not be a pure wrapper class anymore, which diverges from the Tensor-TensorImpl separation design.

I'm not sure what definition you are using for "pure wrapper class", but we are just talking about functions, not state, and variables already have a bunch of extra functions.

gchanan

Just from taking a quick look at this, I think the right thing to do is kind of a split:

set_requires_grad / requires_grad / grad should go on AutogradMeta; these are mostly just accessors/setters anyway so not a big deal.
All the other functions should go on Variable. I don't quite get why they need to be on AutogradMeta.

What do you think?

gchanan · 2018-12-27T00:52:44Z

c10/core/TensorImpl.cpp

+  if (autograd_meta()) {
+    return autograd_meta()->grad();
+  } else {
+    AT_ERROR("grad is not implemented for Tensor");


it sounds like it's correct until we actually merge variable and tensor. It's worth thinking about if this error message will naturally go away in that case, or you'll have to do active work to make it go away. In either case, a FixMe should be here; in the later case, it's probably worth filing an issue so you don't forget.

gchanan · 2018-12-27T01:01:16Z

torch/csrc/autograd/variable.cpp

  std::vector<Variable> inputs;
  if (!gradient.has_value()) {
-    gradient = make_variable(at::ones_like(data_), /*requires_grad=*/false);
+    gradient = make_variable(at::ones_like(data()), /*requires_grad=*/false);


is this future proofing or is there a semantic change here?

There is actually no semantic change - the old backward function lives in Variable::Impl and we can directly access data_, while the new backward function lives in Variable and we need to use either data() or impl_->data_ to access the underlying Tensor data.

In the next PR (when Variable itself becomes the data tensor), we will use *this instead of data() in this line.

…able" This reverts commit 5bafafb.

facebook-github-bot

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@yf225 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Dec 21, 2018

yf225 removed the oncall: jit Add this issue/PR to JIT oncall triage queue label Dec 21, 2018

yf225 requested review from ezyang and gchanan December 21, 2018 20:06

yf225 commented Dec 21, 2018

View reviewed changes

torch/csrc/autograd/variable.cpp Outdated Show resolved Hide resolved

ezyang reviewed Dec 21, 2018

View reviewed changes

ezyang approved these changes Dec 21, 2018

View reviewed changes

gchanan reviewed Dec 21, 2018

View reviewed changes

yf225 force-pushed the tensorimpl_autogradmeta_functions branch from ccb8232 to 4934482 Compare December 26, 2018 20:41

Will Feng added 3 commits December 26, 2018 19:43

Move autograd functions from VariableImpl to AutogradMeta

f83870c

Pass Variable instead of TensorImpl

405e671

Move VariableImpl functions to Variable

afdbc59

yf225 force-pushed the tensorimpl_autogradmeta_functions branch from 4934482 to afdbc59 Compare December 27, 2018 00:44

yf225 mentioned this pull request Dec 27, 2018

Variable/Tensor Merge Proposal #13638

Closed

22 tasks

gchanan reviewed Dec 27, 2018

View reviewed changes

yf225 changed the title ~~Move VariableImpl functions to AutogradMeta~~ Move VariableImpl functions to AutogradMeta and Variable Dec 27, 2018

yf225 force-pushed the tensorimpl_autogradmeta_functions branch 3 times, most recently from 1eb3958 to b7346bd Compare December 27, 2018 05:25

Move set_requires_grad/requires_grad/grad from Tensor to Variable

5bafafb

yf225 force-pushed the tensorimpl_autogradmeta_functions branch from b7346bd to 5bafafb Compare December 27, 2018 05:32

Revert "Move set_requires_grad/requires_grad/grad from Tensor to Vari…

a966c03

…able" This reverts commit 5bafafb.

facebook-github-bot reviewed Dec 27, 2018

View reviewed changes

gchanan approved these changes Dec 27, 2018

View reviewed changes

facebook-github-bot reviewed Dec 28, 2018

View reviewed changes

facebook-github-bot closed this in 9bf7eb9 Dec 28, 2018

ezyang added the merged label Jun 25, 2019

Move VariableImpl functions to AutogradMeta and Variable #15487

Move VariableImpl functions to AutogradMeta and Variable #15487

Uh oh!

Conversation

yf225 commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ezyang Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

yf225 Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

gchanan Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

yf225 Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchanan commented Dec 21, 2018

Uh oh!

gchanan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchanan Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

gchanan Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

yf225 Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yf225 commented Dec 21, 2018 •

edited

Loading

yf225 Dec 27, 2018 •

edited

Loading

gchanan left a comment •

edited

Loading