Retry - [JIT] Propagate profiled information to DifferentiableGraph outputs by davidberard98 · Pull Request #79498 · pytorch/pytorch

davidberard98 · 2022-06-14T03:07:40Z

Stack from ghstack:

-> Retry - [JIT] Propagate profiled information to DifferentiableGraph outputs #79498

Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output.

Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs.

Changes from previous revision: previously, an output node would sometimes be profiled within a no_grad region, meaning that the profiled value would have the wrong requires_grad value (i.e. it would be profiled as requires_grad=False in the no_grad region; but at the time of output, it would have been profiled as requires_grad=True). To fix this, we keep track of which context each node is part of, and only consider profile nodes that are within the same context region. Tested on both of the internal models. (the memory leak and no_grad model).

Differential Revision: D37131946

…utputs Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. [ghstack-poisoned]

facebook-github-bot · 2022-06-14T03:07:45Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/79498
📄 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 our office hours

✅ No Failures (0 Pending)

As of commit 3b1521a (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

…utputs Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. ghstack-source-id: 53436e2 Pull Request resolved: #79498

…ableGraph outputs" Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. [ghstack-poisoned]

davidberard98 · 2022-06-14T04:37:22Z

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

davidberard98 · 2022-06-14T04:50:30Z

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

…ableGraph outputs" Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. Differential Revision: [D37131946](https://our.internmc.facebook.com/intern/diff/D37131946) [ghstack-poisoned]

…utputs Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. ghstack-source-id: 6297ca4 Pull Request resolved: #79498

davidberard98 · 2022-06-14T04:54:19Z

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

davidberard98 · 2022-06-15T19:18:39Z

+    }
+
+    if (!requiresGradResults.empty()) {
+      bool requires_grad = std::any_of(


@eellison does this seem like a safe fix to the nograd issue?

I can think of one case where the output should requires_grad but this won't do so: if all the uses of an diff. graph output that requires_grad are used in nograd sections, then all the profiled values will record requires_grad=False. But I think that's probably fine because then I don't think it matters that it requires_grad?

…ableGraph outputs" Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. Differential Revision: [D37131946](https://our.internmc.facebook.com/intern/diff/D37131946) [ghstack-poisoned]

…utputs Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. ghstack-source-id: f2c6290 Pull Request resolved: #79498

davidberard98 · 2022-06-16T18:35:04Z

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

…ableGraph outputs" Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. Differential Revision: [D37131946](https://our.internmc.facebook.com/intern/diff/D37131946) [ghstack-poisoned]

…utputs Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. ghstack-source-id: 2d2bdf2 Pull Request resolved: #79498

davidberard98 · 2022-06-18T03:39:08Z

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

eellison

nice!!!

davidberard98 · 2022-06-21T22:56:00Z

@pytorchbot merge

pytorchmergebot · 2022-06-21T22:57:12Z

@pytorchbot successfully started a merge job. Check the current status here

pytorchmergebot · 2022-06-21T22:57:19Z

@davidberard98 your PR has been successfully merged.

github-actions · 2022-06-21T22:57:49Z

Hey @davidberard98.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

…utputs (#79498) Summary: Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. Pull Request resolved: #79498 Approved by: https://github.com/eellison Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/268ced51045f59c4a36c32edcad736a754c61288 Reviewed By: eellison Differential Revision: D37131946 Pulled By: davidberard98 fbshipit-source-id: 7dcfa99f65048d59eaf0628f89ecabe123f16a77

…utputs Without profiled outputs, autodiff can't tell whether or not the outputs of a DifferentiableGraph should requires_grad. Autodiff would default to requires_grad=True if there was no profiled information, causing autodiff to mark tensors as requires_grad when they shouldn't have. This adds requires_grad info onto the type of the output, if it can be found in later uses of the output. Adds a test for correct autodiff requires_grad behavior and also a test to make sure the output type is correctly annotated in create_autodiff_subgraphs. Pull Request resolved: pytorch#79498 Approved by: https://github.com/eellison

facebook-github-bot added the cla signed label Jun 14, 2022

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 14, 2022

davidberard98 requested review from Krovatkin and eellison June 15, 2022 19:15

davidberard98 commented Jun 15, 2022

View reviewed changes

eellison approved these changes Jun 21, 2022

View reviewed changes

pytorchmergebot added the Merged label Jun 21, 2022

pytorchmergebot closed this in 268ced5 Jun 21, 2022

facebook-github-bot deleted the gh/davidberard98/115/head branch June 25, 2022 14:16

davidberard98 mentioned this pull request Aug 18, 2022

JIT compiling this specific function with grad-enabled tensors makes it always produce grad-enabled tensors #83378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry - [JIT] Propagate profiled information to DifferentiableGraph outputs#79498

Retry - [JIT] Propagate profiled information to DifferentiableGraph outputs#79498
davidberard98 wants to merge 5 commits intogh/davidberard98/115/basefrom
gh/davidberard98/115/head

davidberard98 commented Jun 14, 2022 •

edited

Loading

Uh oh!

facebook-github-bot commented Jun 14, 2022 •

edited

Loading

Uh oh!

davidberard98 commented Jun 14, 2022

Uh oh!

davidberard98 commented Jun 14, 2022

Uh oh!

davidberard98 commented Jun 14, 2022

Uh oh!

davidberard98 Jun 15, 2022

Uh oh!

davidberard98 commented Jun 16, 2022

Uh oh!

davidberard98 commented Jun 18, 2022

Uh oh!

eellison left a comment

Uh oh!

davidberard98 commented Jun 21, 2022

Uh oh!

pytorchmergebot commented Jun 21, 2022

Uh oh!

pytorchmergebot commented Jun 21, 2022

Uh oh!

github-actions Bot commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

davidberard98 commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

davidberard98 commented Jun 14, 2022

Uh oh!

davidberard98 commented Jun 14, 2022

Uh oh!

davidberard98 commented Jun 14, 2022

Uh oh!

davidberard98 Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

davidberard98 commented Jun 16, 2022

Uh oh!

davidberard98 commented Jun 18, 2022

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

davidberard98 commented Jun 21, 2022

Uh oh!

pytorchmergebot commented Jun 21, 2022

Uh oh!

pytorchmergebot commented Jun 21, 2022

Uh oh!

github-actions Bot commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidberard98 commented Jun 14, 2022 •

edited

Loading

facebook-github-bot commented Jun 14, 2022 •

edited

Loading