deprecate enable_pl_optimizer as it is not restored properly #5244

tchaton · 2020-12-23T10:33:58Z

What does this PR do?

This PR deprecates enable_pl_optimizer and clean everything related to it.

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
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? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

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; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

…PyTorchLightning/pytorch-lightning into bugfix/5165_enable_pl_optimizer

…m/PyTorchLightning/pytorch-lightning into bugfix/5224_not_restored_properly

williamFalcon · 2021-01-08T14:44:42Z

how do people opt out of pl optimizer??

this is a pretty hard change we do that no one can opt out of if we disable this flag. i don't feel comfortable with that.

thoughts @ananthsub

tchaton · 2021-01-08T15:45:29Z

how do people opt out of pl optimizer??

this is a pretty hard change we do that no one can opt out of if we disable this flag. i don't feel comfortable with that.

thoughts @ananthsub

Hey Will,

Currently doing enable_pl_optimizer=False don't opt out from LightningOptimizer.
enable_pl_optimizer=False means optimizer will be the LightningOptimizer one only in the scope of training_loop.optimizer_step call.

If user want to opt out, they can access their optimizer when overriding LightningModule.optimizer_step as follow.

class MyLightningModule(..)
    def optimizer_step(..., optimizer, ....)
        user_optimizer = optimizer.optimizer

I have updated the documentation to show the way out.

Best,
T.C

…m/PyTorchLightning/pytorch-lightning into bugfix/5224_not_restored_properly

edenlightning · 2021-01-08T18:54:57Z

README.md

@@ -225,7 +225,8 @@ with tempfile.NamedTemporaryFile(suffix='.onnx', delete=False) as tmpfile:
 ```python
 class LitAutoEncoder(pl.LightningModule):
    def training_step(self, batch, batch_idx, opt_idx):
-        (opt_a, opt_b) = self.optimizers()
+        # access your optimizers with use_pl_optimizer=False. Default is True
+        (opt_a, opt_b) = self.optimizers(use_pl_optimizer=True)


why do we need it if its the default? i think its more confusing than helpful

So people are aware they can opt out. Do you think it should be removed ?

* update * clean test * still in progress * udpdate test * update * update * resolve flake * add test for zero_grad * update * works without accumulated_grad * update * update * resolve amp * revert back to True * update * clean tests * cleaned out * typo * update test * git repare bug * remove print * udpate * Fix formatting/optimizer imports * Refactor the test for cleanliness * Add vanilla model to the test, better var names * Fixed var names, let's clean up these mock tests * repare test * update test * resolve flake8 * add manual_optimization * update tests * resolve flake8 * add random accumulate_grad_batches * improve test * Update tests/trainer/optimization/test_parity_automatic_optimization.py Co-authored-by: Jirka Borovec <[email protected]> * Update tests/trainer/optimization/test_parity_automatic_optimization.py Co-authored-by: Jirka Borovec <[email protected]> * update * clean tests * correct bug * Apply suggestions from code review * format * adress comments * update on comments * wip * typo * depreceate enable_pl_optimizer * resolve latest bugs * update * resolve merge * add comment * Update pytorch_lightning/core/lightning.py Co-authored-by: Jirka Borovec <[email protected]> * Update tests/deprecated_api/test_remove_1-3.py Co-authored-by: Jirka Borovec <[email protected]> * Update pytorch_lightning/trainer/connectors/optimizer_connector.py Co-authored-by: Jirka Borovec <[email protected]> * Update pytorch_lightning/trainer/trainer.py Co-authored-by: Jirka Borovec <[email protected]> * Update pytorch_lightning/trainer/trainer.py Co-authored-by: Jirka Borovec <[email protected]> * Update tests/trainer/optimization/test_parity_automatic_optimization.py Co-authored-by: Jirka Borovec <[email protected]> * update on comments * update restore * add a property * remove setstate as not needed anymore * update test * provide optimizer to on_before_zero_grad * update on comments * update on comments * Update pytorch_lightning/trainer/trainer.py Co-authored-by: Adrian Wälchli <[email protected]> * Update tests/trainer/optimization/test_parity_automatic_optimization.py Co-authored-by: Adrian Wälchli <[email protected]> * Update tests/trainer/optimization/test_parity_automatic_optimization.py Co-authored-by: Adrian Wälchli <[email protected]> * Update tests/trainer/optimization/test_parity_automatic_optimization.py Co-authored-by: Adrian Wälchli <[email protected]> * mofidy import * update changelog * resolve flake8 * update * update * clean doc Co-authored-by: SeanNaren <[email protected]> Co-authored-by: Ubuntu <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Sean Naren <[email protected]> (cherry picked from commit f2e99d6)

tchaton and others added 30 commits December 17, 2020 08:39

update

fbebccb

clean test

f84085c

still in progress

a309878

udpdate test

ae08761

Merge branch 'master' into bugfix/5165_enable_pl_optimizer

3ef910f

update

f5a5d1e

Merge branch 'bugfix/5165_enable_pl_optimizer' of https://github.com/…

7edec88

…PyTorchLightning/pytorch-lightning into bugfix/5165_enable_pl_optimizer

update

b4181ea

resolve flake

be48064

add test for zero_grad

379d2be

update

fd51f32

works without accumulated_grad

05a838e

update

82c2602

update

386f6d4

resolve amp

b007c9d

Merge branch 'master' into bugfix/5165_enable_pl_optimizer

5007e68

revert back to True

88c5c63

Merge branch 'bugfix/5165_enable_pl_optimizer' of https://github.com/…

3accce3

…PyTorchLightning/pytorch-lightning into bugfix/5165_enable_pl_optimizer

update

7fc56ee

clean tests

8d13893

cleaned out

e7abee6

typo

14475e7

update test

b47db5e

git repare bug

6a79921

remove print

c106828

udpate

85e4e96

Fix formatting/optimizer imports

40f7c54

Refactor the test for cleanliness

e6f9945

Add vanilla model to the test, better var names

9d4fd68

Fixed var names, let's clean up these mock tests

f71ce5d

tchaton added 2 commits January 7, 2021 10:50

Merge branch 'bugfix/5224_not_restored_properly' of https://github.co…

538019f

…m/PyTorchLightning/pytorch-lightning into bugfix/5224_not_restored_properly

Merge branch 'master' into bugfix/5224_not_restored_properly

36e09d5

justusschock approved these changes Jan 7, 2021

View reviewed changes

tchaton added 6 commits January 8, 2021 16:55

update

ca41a18

update

db5e102

Merge branch 'master' into bugfix/5224_not_restored_properly

f6b272c

clean doc

0542c5f

Merge branch 'bugfix/5224_not_restored_properly' of https://github.co…

57a06c1

…m/PyTorchLightning/pytorch-lightning into bugfix/5224_not_restored_properly

Merge branch 'master' into bugfix/5224_not_restored_properly

3374578

williamFalcon approved these changes Jan 8, 2021

View reviewed changes

tchaton enabled auto-merge (squash) January 8, 2021 17:15

tchaton added 2 commits January 8, 2021 18:27

update

2dcfd2a

Merge branch 'bugfix/5224_not_restored_properly' of https://github.co…

b1e847b

…m/PyTorchLightning/pytorch-lightning into bugfix/5224_not_restored_properly

edenlightning reviewed Jan 8, 2021

View reviewed changes

tchaton merged commit f2e99d6 into master Jan 8, 2021

tchaton deleted the bugfix/5224_not_restored_properly branch January 8, 2021 21:13

SeanNaren mentioned this pull request Jan 26, 2021

Regression between Lightning 1.1.3 and 1.1.5 #5656

Closed

indigoviolet mentioned this pull request Mar 2, 2021

Optimizers are broken with auto_lr_find=True since 1.1.4 #6285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate enable_pl_optimizer as it is not restored properly #5244

deprecate enable_pl_optimizer as it is not restored properly #5244

tchaton commented Dec 23, 2020 •

edited

Loading

williamFalcon commented Jan 8, 2021

tchaton commented Jan 8, 2021 •

edited

Loading

edenlightning Jan 8, 2021

tchaton Jan 8, 2021

deprecate enable_pl_optimizer as it is not restored properly #5244

deprecate enable_pl_optimizer as it is not restored properly #5244

Conversation

tchaton commented Dec 23, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

williamFalcon commented Jan 8, 2021

tchaton commented Jan 8, 2021 • edited Loading

edenlightning Jan 8, 2021

Choose a reason for hiding this comment

tchaton Jan 8, 2021

Choose a reason for hiding this comment

tchaton commented Dec 23, 2020 •

edited

Loading

tchaton commented Jan 8, 2021 •

edited

Loading