C++ API parity: at::Tensor::requires_grad_ by pbelevich · Pull Request #26332 · pytorch/pytorch

pbelevich · 2019-09-17T01:52:02Z

Stack from ghstack:

C++ API parity: at::Tensor::register_hook #26568 C++ API parity: at::Tensor::register_hook
C++ API parity: at::Tensor::requires_grad_ #26332 C++ API parity: at::Tensor::requires_grad_

Differential Revision: D17427575

ghstack-source-id: 680c8c6 Pull Request resolved: #26332

yf225

Overall looks awesome! I left a minor comment.

torch/csrc/autograd/VariableTypeManual.cpp

eellison · 2019-09-17T16:13:23Z

This will break things in JIT because we have a pass that analyzes whether or not a Tensor has a gradient or not

yf225 · 2019-09-17T16:40:44Z

@eellison Do you mind elaborating more on the use cases that this will break? The at::Tensor::requires_grad_ API is supposed to be used from the C++ frontend, which I think shouldn't break JIT?

eellison · 2019-09-17T17:04:29Z

@eellison Do you mind elaborating more on the use cases that this will break? The at::Tensor::requires_grad_ API is supposed to be used from the C++ frontend, which I think shouldn't break JIT?

All of the functions in native_functions.yaml are exposed in JIT. We would like to land this in JIT as well just have to update our analysis pass before this lands.

Differential Revision: [D17427575](https://our.internmc.facebook.com/intern/diff/D17427575)

ghstack-source-id: bd597e0 Pull Request resolved: #26332

yf225

Thanks a lot @pbelevich !

yf225

Thanks a lot @pbelevich!

eellison · 2019-10-09T18:27:07Z

@bwasti showed me an example of c10 registration and I added requires_grad_ to torch/csrc/jit/register_prim_ops.cpp

Could you remove the register_prim_ops implementation ? Those are for registering ops that are not bound to the torch c++ library. There is no need to have it in C++ and in register_prim_ops, since the c++ ops are exposed to the JIT already.

bwasti · 2019-10-09T18:34:04Z

torch/csrc/jit/register_prim_ops.cpp

           return 0;
         },
         aliasAnalysisConservative()),
+     Operator(


This looks good and should fix the issue in JIT

bwasti

assuming all tests pass

pbelevich · 2019-10-09T18:45:07Z

@eellison @bwasti I'm very confused with your comments. Should I change anything or not?

bwasti · 2019-10-09T18:49:24Z

@pbelevich edited my comment

bwasti · 2019-10-09T18:52:52Z

@eellison , this one wouldn't be exposed correctly. note the "conservative" annotation in the prim_ops registration.

eellison · 2019-10-09T19:01:04Z

@eellison , this one wouldn't be exposed correctly. note the "conservative" annotation in the prim_ops registration.

Do you know that the register_prim_ops schema is being matched to before the native_functions one?

bwasti · 2019-10-09T19:06:57Z

@eellison that seems to be how it is used in the file,
https://bddppq.github.io/codebrowser/pytorch/pytorch/torch/csrc/jit/register_prim_ops.cpp.html#2823

I don't know if bugs have crept in -- the c10 dispatch code is extremely hard to understand. I believe, if there is an issue, there will be a runtime error
https://bddppq.github.io/codebrowser/pytorch/pytorch/aten/src/ATen/core/dispatch/Dispatcher.cpp.html#63

eellison · 2019-10-09T20:14:19Z

@eellison that seems to be how it is used in the file,
https://bddppq.github.io/codebrowser/pytorch/pytorch/torch/csrc/jit/register_prim_ops.cpp.html#2823

I don't know if bugs have crept in -- the c10 dispatch code is extremely hard to understand. I believe, if there is an issue, there will be a runtime error
https://bddppq.github.io/codebrowser/pytorch/pytorch/aten/src/ATen/core/dispatch/Dispatcher.cpp.html#63

From looking at the code I would suspect this should raise, since it's the same schema with a different options. If doesn't than i would think that's a bug. cc @smessmer

smessmer

lgtm

eellison · 2019-10-14T16:51:47Z

lgtm

@smessmer why is it not a bug that https://bddppq.github.io/codebrowser/pytorch/pytorch/aten/src/ATen/core/dispatch/Dispatcher.cpp.html#63 didn't fire?

smessmer

oh wait I'm a bit confused here. The entry in native_functions.yaml should already create a jit op for this in register_aten_ops.cpp. Why is the one in register_prim_ops.cpp needed?

@eellison This doesn't crash because register_prim_ops.cpp isn't the c10 operator library, that's a shortcut directly to jit which should only be used if absolutely needed.

pbelevich · 2019-10-21T13:25:31Z

@smessmer @bwasti can you please reach a consensus?

smessmer

withdrawing my concerns with recent changes

Differential Revision: [D17427575](https://our.internmc.facebook.com/intern/diff/D17427575) [ghstack-poisoned]

facebook-github-bot · 2019-10-24T20:43:17Z

@pbelevich merged this pull request in 46f96d1.

Summary: Pull Request resolved: pytorch/pytorch#26332 Test Plan: Imported from OSS Differential Revision: D17427575 Pulled By: pbelevich fbshipit-source-id: 5500169a4fa0ef9cc2a7272e13b6e2d89df09260

torch/csrc/autograd/VariableTypeManual.cpp

Summary: Pull Request resolved: pytorch#26332 Test Plan: Imported from OSS Differential Revision: D17427575 Pulled By: pbelevich fbshipit-source-id: 5500169a4fa0ef9cc2a7272e13b6e2d89df09260

wanchaol · 2020-04-03T21:50:42Z

torch/csrc/jit/register_prim_ops.cpp

         },
         aliasAnalysisConservative()),
+     Operator(
+         "aten::requires_grad_(Tensor(a!) self, bool _requires_grad=True) -> Tensor(a!)",


@pbelevich hey any reason we have this schema with _requires_grad instead of requires_grad? This is creating discrepancy between the jit api and the python api as it take requires_grad..

C++ API parity: at::Tensor::requires_grad_

a074fe8

pbelevich requested review from apaszke, ebetica, goldsborough and yf225 as code owners September 17, 2019 01:52

pbelevich mentioned this pull request Sep 17, 2019

[BC-breaking] C++ API parity: at::Tensor::grad #26150

Closed

This was referenced Sep 17, 2019

C++ API parity: at::Tensor::is_leaf #26186

Closed

C++ API parity: at::Tensor::output_nr #26216

Closed

C++ API parity: at::Tensor::version #26217

Closed

C++ API parity: at::Tensor::detach #26251

Closed

pbelevich added a commit that referenced this pull request Sep 17, 2019

C++ API parity: at::Tensor::requires_grad_

4a1a7b7

ghstack-source-id: 680c8c6 Pull Request resolved: #26332

pbelevich removed request for apaszke, ebetica and goldsborough September 17, 2019 01:52

yf225 reviewed Sep 17, 2019

View reviewed changes

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

Update on "C++ API parity: at::Tensor::requires_grad_"

5a5092b

Update on "C++ API parity: at::Tensor::requires_grad_"

0789373

Update on "C++ API parity: at::Tensor::requires_grad_"

bbcb768

Differential Revision: [D17427575](https://our.internmc.facebook.com/intern/diff/D17427575)

pbelevich added a commit that referenced this pull request Sep 17, 2019

C++ API parity: at::Tensor::requires_grad_

b6f002f

ghstack-source-id: bd597e0 Pull Request resolved: #26332

yf225 approved these changes Sep 17, 2019

View reviewed changes

pbelevich requested a review from yf225 October 9, 2019 17:19

yf225 approved these changes Oct 9, 2019

View reviewed changes

bwasti reviewed Oct 9, 2019

View reviewed changes

bwasti approved these changes Oct 9, 2019

View reviewed changes

pbelevich requested a review from smessmer October 10, 2019 14:18

smessmer approved these changes Oct 14, 2019

View reviewed changes

smessmer requested changes Oct 15, 2019

View reviewed changes

smessmer approved these changes Oct 23, 2019

View reviewed changes

pbelevich added 2 commits October 23, 2019 15:37

Update on "C++ API parity: at::Tensor::requires_grad_"

3ccd865

Differential Revision: [D17427575](https://our.internmc.facebook.com/intern/diff/D17427575) [ghstack-poisoned]

Update on "C++ API parity: at::Tensor::requires_grad_"

679aefe

Differential Revision: [D17427575](https://our.internmc.facebook.com/intern/diff/D17427575) [ghstack-poisoned]

facebook-github-bot closed this in 46f96d1 Oct 24, 2019

facebook-github-bot added the merged label Oct 24, 2019

resistor reviewed Oct 28, 2019

View reviewed changes

torch/csrc/autograd/VariableTypeManual.cpp Show resolved Hide resolved

facebook-github-bot deleted the gh/pbelevich/8/head branch October 28, 2019 22:17

gchanan mentioned this pull request Nov 4, 2019

JIT should respect SKIP_PYTHON_BINDINGS and SKIP_PYTHON_BINDINGS_SIGNATURES #29128

Open

wanchaol reviewed Apr 3, 2020

View reviewed changes

mruberry added the Merged label Oct 28, 2020

Conversation

pbelevich commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eellison commented Sep 17, 2019

Uh oh!

yf225 commented Sep 17, 2019

Uh oh!

eellison commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

eellison commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwasti Oct 9, 2019 • 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!

bwasti left a comment

Choose a reason for hiding this comment

Uh oh!

pbelevich commented Oct 9, 2019

Uh oh!

bwasti commented Oct 9, 2019

Uh oh!

bwasti commented Oct 9, 2019

Uh oh!

eellison commented Oct 9, 2019

Uh oh!

bwasti commented Oct 9, 2019

Uh oh!

eellison commented Oct 9, 2019

Uh oh!

smessmer left a comment

Choose a reason for hiding this comment

Uh oh!

eellison commented Oct 14, 2019

Uh oh!

smessmer left a comment

Choose a reason for hiding this comment

Uh oh!

pbelevich commented Oct 21, 2019

Uh oh!

smessmer left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 24, 2019

Uh oh!

Uh oh!

wanchaol Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

pbelevich commented Sep 17, 2019 •

edited

Loading

eellison commented Sep 17, 2019 •

edited

Loading

eellison commented Oct 9, 2019 •

edited

Loading

bwasti Oct 9, 2019 •

edited

Loading