KEMBAR78
[AOTInductor] Enforce no_grad for Run entries by SherlockNoMad · Pull Request #111613 · pytorch/pytorch · GitHub
Skip to content

[AOTInductor] Enforce no_grad for Run entries #111613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[AOTInductor] Enforce no_grad for Run entries #111613

wants to merge 1 commit into from

Conversation

SherlockNoMad
Copy link
Contributor

@SherlockNoMad SherlockNoMad commented Oct 19, 2023 •
edited by pytorch-bot bot
Loading

Summary:
Always enter no_grad mode in AOTInductor run entries.

// AOTInductor uses at::addmm_out, which doesn't supports
// arguments that requires gradient. For this reason, we
// enforce no_grad context for run APIs.

Test Plan:
buck2 test mode/dev-nosan caffe2/test/inductor:test_aot_inductor

and OSS CI

Differential Revision: D50432042

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov @ColinPeppler

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 19, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit fae89ab with merge base 6a99291 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50432042

@SherlockNoMad SherlockNoMad added the topic: not user facing topic category label Oct 19, 2023
khabinov
khabinov reviewed Oct 19, 2023
View reviewed changes
chenyang78
chenyang78 approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@chenyang78 chenyang78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Oleg made a good point regarding recovering from Exceptions. LGTM otherwise. Thanks!

desertfire
desertfire reviewed Oct 20, 2023
View reviewed changes
torch/_inductor/codegen/aoti_runtime/interface.cpp Outdated
// enforce no_grad context for run APIs.
#define WITH_NO_GRAD(...) \
do { \
bool prev_mode = aoti_torch_grad_mode_is_enabled(); \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider print a warning msg if prev_mode is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be invoked in critical path of run(), so I shouldn't print log at every run() call.

I can print warning at ContainerCreate(), but there is a chance that create() is called under no_grad, but run() is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging now.
I can add logs latter if required.

SherlockNoMad added a commit to SherlockNoMad/pytorch that referenced this pull request Oct 23, 2023
@SherlockNoMad @facebook-github-bot
[AOTInductor] Enforce no_grad for Run entries (pytorch#111613)
f08b46d 
Summary:

Always enter no_grad mode in AOTInductor run entries.

```
// AOTInductor uses at::addmm_out, which doesn't supports
// arguments that requires gradient. For this reason, we
// enforce no_grad context for run APIs.
```

Test Plan:
buck2 test mode/dev-nosan caffe2/test/inductor:test_aot_inductor

and OSS CI

Reviewed By: chenyang78

Differential Revision: D50432042
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50432042

@SherlockNoMad
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 23, 2023
@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50432042

@SherlockNoMad
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

SherlockNoMad added a commit to SherlockNoMad/pytorch that referenced this pull request Oct 23, 2023
@SherlockNoMad @facebook-github-bot
[AOTInductor] Enforce no_grad for Run entries (pytorch#111613)
ad7f569 
Summary:

Always enter no_grad mode in AOTInductor run entries.

```
// AOTInductor uses at::addmm_out, which doesn't supports
// arguments that requires gradient. For this reason, we
// enforce no_grad context for run APIs.
```

Test Plan:
buck2 test mode/dev-nosan caffe2/test/inductor:test_aot_inductor

and OSS CI

Reviewed By: khabinov, chenyang78

Differential Revision: D50432042
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50432042

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50432042

@SherlockNoMad @facebook-github-bot
[AOTInductor] Enforce no_grad for Run entries (pytorch#111613)
fae89ab 
Summary:

Always enter no_grad mode in AOTInductor run entries.

```
// AOTInductor uses at::addmm_out, which doesn't supports
// arguments that requires gradient. For this reason, we
// enforce no_grad context for run APIs.
```

Test Plan:
buck2 test mode/dev-nosan caffe2/test/inductor:test_aot_inductor

and OSS CI

Reviewed By: khabinov, chenyang78

Differential Revision: D50432042
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50432042

@SherlockNoMad
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Nov 7, 2023
@SherlockNoMad @xuhancn
[AOTInductor] Enforce no_grad for Run entries (pytorch#111613)
7b12353 
Summary:
Always enter no_grad mode in AOTInductor run entries.

```
// AOTInductor uses at::addmm_out, which doesn't supports
// arguments that requires gradient. For this reason, we
// enforce no_grad context for run APIs.
```

Test Plan:
buck2 test mode/dev-nosan caffe2/test/inductor:test_aot_inductor

and OSS CI

Differential Revision: D50432042

Pull Request resolved: pytorch#111613
Approved by: https://github.com/chenyang78, https://github.com/khabinov
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
@SherlockNoMad @Skylion007
[AOTInductor] Enforce no_grad for Run entries (pytorch#111613)
8218dbb 
Summary:
Always enter no_grad mode in AOTInductor run entries.

```
// AOTInductor uses at::addmm_out, which doesn't supports
// arguments that requires gradient. For this reason, we
// enforce no_grad context for run APIs.
```

Test Plan:
buck2 test mode/dev-nosan caffe2/test/inductor:test_aot_inductor

and OSS CI

Differential Revision: D50432042

Pull Request resolved: pytorch#111613
Approved by: https://github.com/chenyang78, https://github.com/khabinov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@desertfire desertfire desertfire left review comments

@chenyang78 chenyang78 chenyang78 approved these changes

+1 more reviewer

@khabinov khabinov khabinov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged module: inductor topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@SherlockNoMad @facebook-github-bot @pytorchmergebot @chenyang78 @khabinov @desertfire