Skip to content
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.

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

Already on GitHub? Sign in to your account

There is no interation between no_grad, enable_grad, set_grad_enabled #40158

Closed
Yura52 opened this issue Jun 17, 2020 · 5 comments
Closed

There is no interation between no_grad, enable_grad, set_grad_enabled #40158

Yura52 opened this issue Jun 17, 2020 · 5 comments
Assignees
Labels
high priority module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks small We think this is a small issue to fix. Consider knocking off high priority small issues triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@Yura52
Copy link
Contributor

Yura52 commented Jun 17, 2020

馃摎 Documentation

torch==1.5.0

The problem is the same for all three functions. For example, let's consider no_grad.

The documentation says: "This mode has no effect when using enable_grad context manager". This is not true:

import torch
x = torch.tensor([1.0], requires_grad=True)
with torch.enable_grad():
    with torch.no_grad():
        y = x * 2
assert not y.requires_grad

And I mean that the documentation should be fixed, not the behavior of torch 馃槃
These functions just set grad enabled/disabled in __enter__ (or in the constructor in the case of set_grad_enabled) and set the previous state in __exit__. This is very intuitive. IMHO, it will be confusing if, for example, the behavior of no_grad changes depending on some other context.

cc @ezyang @gchanan @zou3519 @ssnl @albanD @gqchen @jlin27

@ailzhang ailzhang added module: autograd Related to torch.autograd, and the autograd engine in general triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Jun 17, 2020
@colesbury
Copy link
Member

This was changed in #23310 apparently because of #19189.

The complaint in #19189 was legitimate -- the original terminology was confusing. But the PR #23310 confuses things by also adding erroneous claims:

https://github.com/pytorch/pytorch/pull/23310/files#diff-a1d32fe3effc8d03f2cf3e8af9216de9R106-R107

https://github.com/pytorch/pytorch/pull/23310/files#diff-a1d32fe3effc8d03f2cf3e8af9216de9R15

We should have caught that in the PR review. Oops.

@colesbury
Copy link
Member

I'm bumping this to high priority considering @ezyang marked #19189 high-pri. The current documentation is incorrect. We should remove these two statements:

This mode has no effect when using :class:`~enable_grad` context manager .
    When using :class:`~enable_grad` context manager, :class:`~set_grad_enabled(False)`
    has no effect.

@colesbury colesbury added the module: docs Related to our documentation, both in docs/ and docblocks label Jun 17, 2020
@ezyang ezyang added small We think this is a small issue to fix. Consider knocking off high priority small issues and removed triage review labels Jun 18, 2020
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jun 26, 2020

@colesbury just to confirm, this issue is about documentation only (no code change required), right ?

vfdev-5 pushed a commit to Quansight/pytorch that referenced this issue Jun 26, 2020
- docs update, removed incorrect statements
@colesbury
Copy link
Member

@vfdev-5 yes, documentation only

@peterbell10
Copy link
Collaborator

@vfdev-5 friendly reminder to assign yourself to issues you're working on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks small We think this is a small issue to fix. Consider knocking off high priority small issues triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants