[bugfix] Resolve bug with multiple optimizers and toggle. #5574

tchaton · 2021-01-19T14:51:59Z

What does this PR do?

In case of multiple optimizers, toggle optimizer was called.
It would lose user requieres_grad state by setting everything to True or False.

This is erroneous in case there are any parameters which are explicitly kept non-trainable.

Fixes #5292 #5642

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified
Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2021-01-19T14:52:08Z

Hello @tchaton! Thanks for updating this PR.

In the file tests/core/test_lightning_module.py:

Line 183:121: E501 line too long (150 > 120 characters)

Comment last updated at 2021-01-25 20:48:57 UTC

…ytorch-lightning into bugfix/toggle

pytorch_lightning/core/lightning.py

rohitgr7 · 2021-01-19T18:19:47Z

pytorch_lightning/core/lightning.py

@@ -1166,16 +1166,44 @@ def toggle_optimizer(self, optimizer: Optimizer, optimizer_idx: int):



maybe just use the opt_idx to get the optimizer down there... just to unify the arguments with untoggle_optimizer. Either way is fine.

Sounds great ! I will do so.

Do you plan to do it in this PR?

@tchaton ^^

@rohitgr7 @carmocca @tchaton please see #5775

codecov · 2021-01-19T18:49:06Z

Codecov Report

Merging #5574 (9f11c76) into master (e87424a) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #5574   +/-   ##
======================================
- Coverage      93%     93%   -0%     
======================================
  Files         135     135           
  Lines       10034   10051   +17     
======================================
+ Hits         9369    9383   +14     
- Misses        665     668    +3

pytorch_lightning/core/lightning.py

tests/core/test_lightning_module.py

pytorch_lightning/core/lightning.py

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

tests/core/test_lightning_module.py

pytorch_lightning/core/lightning.py

…ytorch-lightning into bugfix/toggle

CHANGELOG.md

* fix toggle_optimizer * update doc * resolve bug * update * Update pytorch_lightning/core/lightning.py Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * update on comments * update on comments * update Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> (cherry picked from commit c76cc23)

tchaton added 2 commits January 19, 2021 14:38

fix toggle_optimizer

0e9b03e

update doc

400530d

tchaton requested review from awaelchli, Borda, justusschock, SeanNaren and williamFalcon as code owners January 19, 2021 14:52

tchaton self-assigned this Jan 19, 2021

tchaton added this to the 1.1.x milestone Jan 19, 2021

tchaton added bug Something isn't working priority: 1 Medium priority task labels Jan 19, 2021

Merge branch 'master' into bugfix/toggle

be9dc6c

tchaton mentioned this pull request Jan 19, 2021

Fix grad getting enabled for parameters on which explicitly disabled in case of multiple optimizers #5356

Closed

12 tasks

carmocca self-requested a review January 19, 2021 15:05

tchaton added 4 commits January 19, 2021 16:36

resolve bug

b25921a

Merge branch 'bugfix/toggle' of https://github.com/PyTorchLightning/p…

df1a398

…ytorch-lightning into bugfix/toggle

update

8252877

Merge branch 'master' into bugfix/toggle

fbb9c44

rohitgr7 approved these changes Jan 19, 2021

View reviewed changes

carmocca reviewed Jan 19, 2021

View reviewed changes

tchaton and others added 2 commits January 19, 2021 19:32

Update pytorch_lightning/core/lightning.py

64314fa

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

update on comments

be15bf1

carmocca reviewed Jan 20, 2021

View reviewed changes

tests/core/test_lightning_module.py Outdated Show resolved Hide resolved

carmocca reviewed Jan 20, 2021

View reviewed changes

pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved

tchaton added 2 commits January 20, 2021 19:55

update on comments

c195e4c

Merge branch 'master' into bugfix/toggle

dd6b5d3

Borda reviewed Jan 20, 2021

View reviewed changes

pytorch_lightning/core/lightning.py Show resolved Hide resolved

carmocca approved these changes Jan 20, 2021

View reviewed changes

tchaton added 2 commits January 20, 2021 20:33

update

beb4274

Merge branch 'bugfix/toggle' of https://github.com/PyTorchLightning/p…

af95dae

…ytorch-lightning into bugfix/toggle

tchaton enabled auto-merge (squash) January 21, 2021 08:13

tchaton added 2 commits January 21, 2021 08:14

update changelog

eda5e44

update changelog

2034093

justusschock approved these changes Jan 21, 2021

View reviewed changes

SeanNaren reviewed Jan 21, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

tchaton added 2 commits January 22, 2021 09:25

update changelog

1d36cc9

Merge branch 'master' into bugfix/toggle

86bcb69

SeanNaren approved these changes Jan 22, 2021

View reviewed changes

mergify bot added 3 commits January 25, 2021 00:31

Merge branch 'master' into bugfix/toggle

71207ad

Merge branch 'master' into bugfix/toggle

e802ae1

Merge branch 'master' into bugfix/toggle

eac3340

tchaton added the ready PRs ready to be merged label Jan 25, 2021

mergify bot added the has conflicts label Jan 25, 2021

Merge branch 'master' into bugfix/toggle

75af9e8

mergify bot removed the has conflicts label Jan 25, 2021

williamFalcon approved these changes Jan 25, 2021

View reviewed changes

Merge branch 'master' into bugfix/toggle

9f11c76

tchaton merged commit c76cc23 into master Jan 25, 2021

tchaton deleted the bugfix/toggle branch January 25, 2021 23:45

phammanhhiep mentioned this pull request Jul 5, 2021

RuntimeError: element 0 of tensors does not require grad and does not have a grad_fn #8281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Resolve bug with multiple optimizers and toggle. #5574

[bugfix] Resolve bug with multiple optimizers and toggle. #5574

tchaton commented Jan 19, 2021 •

edited

Loading

pep8speaks commented Jan 19, 2021 •

edited

Loading

rohitgr7 Jan 19, 2021

tchaton Jan 19, 2021

carmocca Jan 20, 2021

rohitgr7 Jan 25, 2021

ananthsub Feb 4, 2021

codecov bot commented Jan 19, 2021 •

edited

Loading

		@@ -1166,16 +1166,44 @@ def toggle_optimizer(self, optimizer: Optimizer, optimizer_idx: int):

[bugfix] Resolve bug with multiple optimizers and toggle. #5574

[bugfix] Resolve bug with multiple optimizers and toggle. #5574

Conversation

tchaton commented Jan 19, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Jan 19, 2021 • edited Loading

Comment last updated at 2021-01-25 20:48:57 UTC

rohitgr7 Jan 19, 2021

Choose a reason for hiding this comment

tchaton Jan 19, 2021

Choose a reason for hiding this comment

carmocca Jan 20, 2021

Choose a reason for hiding this comment

rohitgr7 Jan 25, 2021

Choose a reason for hiding this comment

ananthsub Feb 4, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2021 • edited Loading

Codecov Report

tchaton commented Jan 19, 2021 •

edited

Loading

pep8speaks commented Jan 19, 2021 •

edited

Loading

codecov bot commented Jan 19, 2021 •

edited

Loading