Move backward and set_data off of Type #21963

li-roy · 2019-06-19T10:05:32Z

Stack from ghstack:

Remove Type dispatch #21964 Remove Type dispatch
Move backward and set_data off of Type #21963 Stop using Type in Python bindings
Remove many usages of Type #21941 Remove many usages of Type
Remove getDeviceFromPtr and allocator from Type #21940 Remove getDeviceFromPtr and allocator from Type
Move UnsafeTensorFromTH and UnsafeStorageFromTH off Type #21923 Move UnsafeTensorFromTH and UnsafeStorageFromTH off Type
[BC-BREAKING] Replace Type dispatch with ATenDispatch #21320 [BC-BREAKING] Replace Type dispatch with ATenDispatch

These functions only work on variables; calling it on a Tensor errors out. I moved the Tensor error stubs to native functions, and changed the variable implementations to be manual overrides in VariableType.

Differential Revision: D15897423

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

ezyang · 2019-06-20T14:00:13Z

aten/src/ATen/core/Tensor.h


  //example
  //Tensor * add(Tensor & b);
+  void backward(const Tensor & gradient={}, bool keep_graph=false, bool create_graph=false) const;


I think the original signature was intentionally optional<Tensor>, to make it clear that passing nullopt is valid (it's not obviously valid for other cases.) Does the codegen choke on this?

we've been planning to make Tensor? translate to optional<Tensor> for awhile now -- maybe now is a good time to do it?

Yeah this doesn't work on codegen yet.

ezyang

The signature change to backward seems harmless enough since it was defaulted, but I would like to understand a little more. Everything else looks fine.

gchanan · 2019-06-20T17:20:22Z

torch/csrc/autograd/variable.cpp

  std::vector<Variable> inputs;
-  if (!gradient.has_value()) {
-    gradient = at::ones_like(*this);
+  Tensor gradient_;


wait, where do you assign gradient to gradient_?

gchanan · 2019-06-20T17:23:17Z

torch/csrc/autograd/python_variable.cpp

-  }
-
-  THPUtils_assertRet(-1, grad.type() == var.type() || gradIsSparse,
+  THPUtils_assertRet(-1, grad.type() == var.type() || grad.is_sparse(),


unless I'm missing something, this doesn't look equivalent? The new code allows you to assign a sparse gradient, even if it's the wrong dtype.

gchanan · 2019-06-20T17:25:02Z

torch/csrc/autograd/VariableTypeManual.cpp

+    const Tensor& gradient,
    bool keep_graph,
-    bool create_graph) const {
+    bool create_graph) {


is there some reason the const disappeared on this backward and set_data?

They are static functions now.

gchanan · 2019-06-20T17:35:09Z

aten/src/ATen/core/TensorMethods.h

+// all static inline to allow for inlining of the non-dynamic part of dispatch
+inline void Tensor::backward(const Tensor & gradient, bool keep_graph, bool create_graph) const {
+    static auto table = globalATenDispatch().getOpTable("aten::backward(Tensor(a!) self, Tensor? gradient=None, bool keep_graph=False, bool create_graph=False) -> void");
+    return table->getOp<void (const Tensor &, const Tensor &, bool, bool)>(tensorTypeIdToBackend(type_id()), is_variable())(*this, gradient, keep_graph, create_graph);


gchanan · 2019-06-20T17:36:44Z

aten/src/ATen/native/native_functions.yaml

 - func: _cast_Half(Tensor self, bool non_blocking=False) -> Tensor
  variants: function

+- func: backward(Tensor(a!) self, Tensor? gradient=None, bool keep_graph=False, bool create_graph=False) -> void


how did you choose these annotations? For example, why is backward's self marked as writeable, but set_data isn't?

Oh I guess I have these reversed. I guess technically set_data should be set_data_, but not sure I want to make that change here.

I'm not even really sure what these annotations mean in this context, as they are pretty different than the usual meanings.

For example, backward is the only place where we mutate self.grad -- does that count as mutable? self itself doesn't actually change.

Maybe @suo can give some guidance here.

backward() is a special case that isn't expressible in the alias annotation language—it writes to every tensor in the autograd graph as the gradients are accumulated to the leaves. So if we enabled in the JIT it would have to be a special case in the alias analysis pass.

The bit about mutating the grad property only isn't that relevant—for the purpose of optimization, you can't reorder a call to backward() across a read of any tensor in the autograd graph anyway, so it's fine to consider self.grad part of the self's memory.

gchanan

see comments.

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

facebook-github-bot · 2019-06-30T11:32:41Z

@li-roy merged this pull request in 6c454ff.

Summary: Pull Request resolved: pytorch/pytorch#21963 ghimport-source-id: 4d9d66ba2c8587503d892b67f535cc2a62e2d19e Test Plan: Imported from OSS Differential Revision: D15897423 Pulled By: li-roy fbshipit-source-id: 2dd55ceb80971df7c86545b7bfff733387f13572

Summary: Pull Request resolved: pytorch#21963 ghimport-source-id: 4d9d66b Test Plan: Imported from OSS Differential Revision: D15897423 Pulled By: li-roy fbshipit-source-id: 2dd55ceb80971df7c86545b7bfff733387f13572

soumyarooproy · 2019-07-23T22:21:12Z

torch/csrc/autograd/python_variable.cpp

-       gradIsSparse = gradType == sparseType;
-  }
-
+  bool gradIsSparse = var.dtype() == grad.dtype() && toSparse(tensorTypeIdToBackend(var.type_id())) == tensorTypeIdToBackend(grad.type_id());


@li-roy this change is breaking backend extensions in that when the var backend type is a backend extension type, toSparse() just throws a runtime error. https://github.com/pytorch/pytorch/blob/master/c10/core/Backend.h#L31

Stop using Type in Python bindings

df221af

li-roy requested review from ebetica, goldsborough and yf225 as code owners June 19, 2019 10:05

This was referenced Jun 19, 2019

Move UnsafeTensorFromTH and UnsafeStorageFromTH off Type #21923

Closed

Remove getDeviceFromPtr and allocator from Type #21940

Closed

Remove many usages of Type #21941

Closed

Remove Type dispatch #21964

Closed

royboy added 2 commits June 19, 2019 10:46

Update on "Stop using Type in Python bindings"

0456097

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Stop using Type in Python bindings"

b73a9c6

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

li-roy changed the title ~~Stop using Type in Python bindings~~ Move backward and set_data off of Type Jun 19, 2019

li-roy requested review from dzhulgakov, ezyang, gchanan and smessmer June 19, 2019 19:28

ezyang reviewed Jun 20, 2019

View reviewed changes

ezyang approved these changes Jun 20, 2019

View reviewed changes

gchanan reviewed Jun 20, 2019

View reviewed changes

gchanan requested changes Jun 20, 2019

View reviewed changes

royboy added 9 commits June 21, 2019 15:27

Update on "Move backward and set_data off of Type"

c9236e3

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

db7b539

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

f979cf2

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

38504e0

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

ae29f78

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

d625561

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

87eb996

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

0b2580c

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

ff87151

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

li-roy mentioned this pull request Jun 24, 2019

Remove Type dispatch #22152

Closed

pytorchbot added the module: typing Related to mypy type annotations label Jun 24, 2019

royboy added 10 commits June 24, 2019 12:54

Update on "Move backward and set_data off of Type"

81913bb

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

f3aa351

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

21adc45

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

297f311

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

1df0016

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

22692e8

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

d27e622

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

0abf2b3

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

0128bf5

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

Update on "Move backward and set_data off of Type"

0bdbe22

Move backward and set_data off of Type gh-metadata: pytorch pytorch 21963 gh/li-roy/31/head

facebook-github-bot closed this in 6c454ff Jun 30, 2019

zou3519 deleted the gh/li-roy/31/head branch June 30, 2019 11:14

facebook-github-bot added the merged label Jun 30, 2019

soumyarooproy reviewed Jul 23, 2019

View reviewed changes

mruberry added the Merged label Oct 28, 2020

Move backward and set_data off of Type #21963

Move backward and set_data off of Type #21963

Uh oh!

Conversation

li-roy commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchanan left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

li-roy commented Jun 19, 2019 •

edited

Loading