Disable `lr_scheduler.step()` in manual optimization by akihironitta · Pull Request #6825 · Lightning-AI/pytorch-lightning

akihironitta · 2021-04-04T21:38:29Z

What does this PR do?

Fixes #6379. Once #6567 and this PR are merged, users can call lr_scheduler.step() at arbitrary intervals in manual optimization for ultimate flexibility.

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

Did you have fun?

Make sure you had fun coding 🙃

codecov · 2021-04-04T21:40:05Z

Codecov Report

Merging #6825 (0ff4fea) into master (d1529c2) will decrease coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #6825    +/-   ##
=======================================
- Coverage      92%     91%    -1%     
=======================================
  Files         197     197            
  Lines       12588   12821   +233     
=======================================
+ Hits        11610   11696    +86     
- Misses        978    1125   +147

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

tchaton · 2021-04-06T07:10:15Z

            monitor_metrics: dict of possible values to monitor
        """
-        if not self.trainer.lr_schedulers:
+        if not self.trainer.lr_schedulers or not self.trainer.train_loop.automatic_optimization:


Let's update the doc with this change + add a warning if we detect the scheduler was on step or epoch, but never triggered.

@tchaton I updated the docs and added a warning when scheduler keys (e.g. "interval", "frequency", "monitor"), which are invalid in manual optimization, are provided. Would you mind having a look at the changes?

Looks good. Could we add in the doc how to use the scheduler for interval=epoch.

def training_step(self, batch, batch_idx) # do optimization if self.trainer.is_last_batch: # is_last_batch doesn't exist for the trainer, could be added. self.lr_schedulers[0].step()

@tchaton I added self.trainer.is_last_batch and updated the docs. Could you have a look again?

Co-authored-by: thomas chaton <thomas@grid.ai>

tchaton

Great work there !

tchaton · 2021-04-09T09:29:12Z

@akihironitta. There is a bug with Pipe.

Better to access trainer.train_loop.automatic_optimization

akihironitta · 2021-04-09T10:20:01Z

I think the failing tests on Windows are not related to the changes here as they're also failing on master.

tests/trainer/test_data_loading.py::test_dataloader_warnings[0] FAILED   [ 63%]
tests/trainer/test_data_loading.py::test_dataloader_warnings[1] FAILED   [ 63%]

Borda

seems like all Windows tests are failing

carmocca · 2021-04-09T13:05:17Z

Windows errors are caused by #6762

akihironitta · 2021-04-09T20:04:29Z

@Borda The failing windows tests should be fixed now by @carmocca in #6930, so would you mind having a look again?

akihironitta · 2021-04-19T22:15:17Z

Just for the note, as #6907 has been merged already, there's no need to change any docs here, so I reverted the docs change.

Windows tests are fixed

akihironitta added 4 commits April 5, 2021 05:44

Add test for scheduler.step in manual optimization

d8a0973

Update the test

761ca27

Disable lr_scheduler.step() in manual optimization

df44b16

Add comment to test

d8de6b0

akihironitta added the feature Is an improvement or enhancement label Apr 4, 2021

akihironitta added this to the 1.3 milestone Apr 4, 2021

Update CHANGLOG

fe267a8

akihironitta changed the title ~~Disable lr_scheduler.step() in manual optimization [WIP]~~ Disable lr_scheduler.step() in manual optimization Apr 5, 2021

akihironitta marked this pull request as ready for review April 5, 2021 09:20

akihironitta requested review from Borda, SeanNaren, awaelchli, carmocca, justusschock, tchaton and williamFalcon as code owners April 5, 2021 09:20

akihironitta mentioned this pull request Apr 5, 2021

[RFC] Add self.lr_schedulers() to LightningModule for manual optimization #6567

Merged

10 tasks

awaelchli reviewed Apr 5, 2021

View reviewed changes

Comment thread tests/trainer/optimization/test_manual_optimization.py Outdated

Comment thread tests/trainer/optimization/test_manual_optimization.py Outdated

Comment thread tests/trainer/optimization/test_manual_optimization.py

akihironitta and others added 2 commits April 5, 2021 19:16

Add training_step to TestModel

bf58e9c

Apply suggestions from code review

4ef09b5

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

tchaton reviewed Apr 6, 2021

View reviewed changes

akihironitta and others added 2 commits April 7, 2021 04:10

Fix typo

80eea22

Co-authored-by: thomas chaton <thomas@grid.ai>

Update docs

6d263f0

akihironitta requested a review from edenlightning as a code owner April 6, 2021 19:42

Add warning when invalid keys are specified

523f2e8

akihironitta marked this pull request as draft April 7, 2021 15:34

akihironitta added 2 commits April 8, 2021 00:50

Update docs

ebf6a3a

Update docs

1e587f0

akihironitta commented Apr 7, 2021

View reviewed changes

Comment thread docs/source/common/optimizers.rst Outdated

akihironitta requested a review from kaushikb11 as a code owner April 8, 2021 19:15

akihironitta mentioned this pull request Apr 8, 2021

Optimization docs #6907

Merged

7 tasks

tchaton approved these changes Apr 9, 2021

View reviewed changes

Comment thread CHANGELOG.md

Comment thread docs/source/common/optimizers.rst Outdated

Comment thread pytorch_lightning/trainer/training_loop.py

mergify Bot added the has conflicts label Apr 9, 2021

akihironitta added 3 commits April 9, 2021 18:42

Update CHANGELOG

97cdb6d

Use train_loop.automatic_optimization

32445d7

Merge branch 'master' into feat/disable-lrscheduler-in-manopt

66b45ff

mergify Bot removed the has conflicts label Apr 9, 2021

akihironitta marked this pull request as draft April 9, 2021 10:06

akihironitta marked this pull request as ready for review April 9, 2021 10:18

akihironitta requested a review from tchaton April 9, 2021 10:22

Borda previously requested changes Apr 9, 2021

View reviewed changes

Merge branch 'master' into feat/disable-lrscheduler-in-manopt

388e4b5

akihironitta requested a review from Borda April 9, 2021 20:04

edenlightning modified the milestones: 1.3, 1.4 Apr 16, 2021

tchaton added 2 commits April 19, 2021 15:09

Merge branch 'master' into feat/disable-lrscheduler-in-manopt

ec3b31f

resolve merge

c457908

tchaton modified the milestones: v1.4, v1.3 Apr 19, 2021

carmocca enabled auto-merge (squash) April 19, 2021 15:57

Revert doc change

0ff4fea

carmocca merged commit 0302b8b into master Apr 20, 2021

carmocca deleted the feat/disable-lrscheduler-in-manopt branch April 20, 2021 11:00

Conversation

akihironitta commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

PR review

Did you have fun?

Uh oh!

codecov Bot commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchaton Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

akihironitta Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchaton Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

akihironitta Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchaton commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akihironitta commented Apr 9, 2021

Uh oh!

Borda left a comment

Choose a reason for hiding this comment

Uh oh!

carmocca commented Apr 9, 2021

Uh oh!

akihironitta commented Apr 9, 2021

Uh oh!

akihironitta commented Apr 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

akihironitta commented Apr 4, 2021 •

edited

Loading

codecov Bot commented Apr 4, 2021 •

edited

Loading

akihironitta Apr 7, 2021 •

edited

Loading

akihironitta Apr 8, 2021 •

edited

Loading

tchaton commented Apr 9, 2021 •

edited

Loading