Fix prod double backward when there are 2+ zeros #113969

guilhermeleobas · 2023-11-17T18:58:24Z

Stack from ghstack (oldest at bottom):

-> Fix prod double backward when there are 2+ zeros #113969

cc @ezyang @albanD @zou3519 @gqchen @pearu @nikitaved @soulitzer @lezcano @Varal7

[ghstack-poisoned]

pytorch-bot · 2023-11-17T18:58:28Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113969

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit fe78741 with merge base 0d6d97d ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[ghstack-poisoned]

ghstack-source-id: 8ee3db3 Pull Request resolved: #113969

albanD

Any details?
Also this is most likely going to be a challenging change just for perf reasons.

guilhermeleobas · 2023-11-18T17:28:12Z

@albanD, motivation for this one is #106789

albanD

Ho ok!
You can update the condition to only be used when double backward is not used then (see condition below)
You can also add a small comment to that effect: when setting up for double backward, we must do the hard work of properly computing the result even though we know it is going to all 0s to ensure the autograd graph is properly created.

albanD · 2023-11-20T15:37:38Z

torch/csrc/autograd/FunctionsManual.cpp

  Tensor zero_idx = (input == 0).nonzero();
  if (zero_idx.sym_numel() == 0) {
    return grad * (result / input).conj();
-  } else if (zero_idx.size(0) > 1) {


(!at::GradMode::is_enabled() && zero_idx.size(0) > 1)

Thanks for the tip, @albanD. I've updated the code.

[ghstack-poisoned]

ghstack-source-id: 68b2b58 Pull Request resolved: #113969

albanD

Thanks!
I would expect that there is some test that used to fail that is now passing?

guilhermeleobas · 2023-11-20T20:47:54Z

@albanD, yes! The following fails on PyTorch main but works on this branch:

import torch

x = torch.tensor([2., 3, 0, 0], requires_grad=True)
y = torch.cumprod(x, dim=0)
gx, = torch.autograd.grad(y.sum(), x, create_graph=True)
gy = torch.autograd.grad(gx.sum(), x)
print(gy)

albanD · 2023-11-20T21:22:36Z

Ho I meant there should be a test in CI running this.

guilhermeleobas · 2023-11-20T21:27:12Z

The extra input I added on common_method_invocarions should do it, no?

Edit: I will send in a bit the test that would fail without the changes for this input.

python test/test_ops_gradients.py -k test_fn_gradgrad_prod_cpu_float64

albanD

The extra input I added on common_method_invocarions should do it, no?

That was my question :p So we already do have gradgradcheck check for this input and so adding this input without the update in c++ would fail?
If so, then SGTM!

guilhermeleobas · 2023-11-20T23:04:51Z

The extra input I added on common_method_invocarions should do it, no?

That was my question :p So we already do have gradgradcheck check for this input and so adding this input without the update in c++ would fail? If so, then SGTM!

Yep! python test/test_ops_gradients.py -k test_fn_gradgrad_prod_cpu_float64

Thanks for the review, @albanD. I'll merge this one.

guilhermeleobas · 2023-11-20T23:04:57Z

@pytorchbot merge

pytorchmergebot · 2023-11-20T23:06:48Z

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team

Raised by workflow job

guilhermeleobas · 2023-11-20T23:23:00Z

@pytorchbot merge

pytorchmergebot · 2023-11-20T23:25:00Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Fix prod double backward when there are 2+ zeros

cf08f8f

[ghstack-poisoned]

pytorchbot added the open source label Nov 17, 2023

Update on "Fix prod double backward when there are 2+ zeros"

871b89d

[ghstack-poisoned]

guilhermeleobas added a commit that referenced this pull request Nov 17, 2023

Fix prod double backward when there are 2+ zeros

7146acb

ghstack-source-id: 8ee3db3 Pull Request resolved: #113969

guilhermeleobas marked this pull request as ready for review November 18, 2023 00:18

guilhermeleobas requested review from albanD, mruberry, ngimel and soulitzer as code owners November 18, 2023 00:18

albanD requested changes Nov 18, 2023

View reviewed changes

albanD reviewed Nov 20, 2023

View reviewed changes

Update on "Fix prod double backward when there are 2+ zeros"

fe78741

[ghstack-poisoned]

guilhermeleobas added a commit that referenced this pull request Nov 20, 2023

Fix prod double backward when there are 2+ zeros

46477ee

ghstack-source-id: 68b2b58 Pull Request resolved: #113969

albanD reviewed Nov 20, 2023

View reviewed changes

albanD approved these changes Nov 20, 2023

View reviewed changes

soulitzer removed their request for review November 20, 2023 22:34

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 20, 2023

pytorchmergebot added the merging label Nov 20, 2023

pytorchmergebot removed the merging label Nov 20, 2023

guilhermeleobas added module: autograd Related to torch.autograd, and the autograd engine in general and removed module: autograd Related to torch.autograd, and the autograd engine in general labels Nov 20, 2023

guilhermeleobas added the release notes: autograd release notes category label Nov 20, 2023

pytorchmergebot added the merging label Nov 20, 2023

pytorchmergebot added the Merged label Nov 21, 2023

pytorchmergebot closed this in 77f16eb Nov 21, 2023

pytorchmergebot removed the merging label Nov 21, 2023

facebook-github-bot deleted the gh/guilhermeleobas/13/head branch November 24, 2023 15:27

Fix prod double backward when there are 2+ zeros #113969

Fix prod double backward when there are 2+ zeros #113969

Uh oh!

Conversation

guilhermeleobas commented Nov 17, 2023 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113969

✅ No Failures

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas commented Nov 18, 2023

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas commented Nov 20, 2023

Uh oh!

albanD commented Nov 20, 2023

Uh oh!

guilhermeleobas commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

guilhermeleobas commented Nov 20, 2023

Uh oh!

guilhermeleobas commented Nov 20, 2023

Uh oh!

pytorchmergebot commented Nov 20, 2023

Merge failed

Uh oh!

guilhermeleobas commented Nov 20, 2023

Uh oh!

pytorchmergebot commented Nov 20, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guilhermeleobas commented Nov 17, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Nov 17, 2023 •

edited

Loading

guilhermeleobas commented Nov 20, 2023 •

edited

Loading