Bugfix/5487 auto lr ordering #5638

noamzilo · 2021-01-24T14:47:00Z

What does this PR do?

before: auto learning rate finder fails AFTER optimization due to misconfiguration
after: auto learning rate finder fails BEFORE optimization due to misconfiguration, and user can fail faster.

Fixes #5487

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 🙃

…slowly

pep8speaks · 2021-01-24T14:47:04Z

Hello @noamzilo! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-01 13:09:50 UTC

…mzilo/pytorch-lightning into bugfix/5487_auto_lr_ordering

noamzilo · 2021-01-24T15:16:27Z

To the maintainers:

I have added a test that requires pip install pytest-timeout

Currently, I am getting the error

_______________ ERROR collecting tests/trainer/test_lr_finder.py _______________
'timeout' not found in markers configuration option
_______________ ERROR collecting tests/trainer/test_lr_finder.py _______________
'timeout' not found in markers configuration option

Will it be possible to install this library so the test will pass?

If not, please recommend what I should use instead so I will be able to change the code.

carmocca · 2021-01-24T23:31:11Z

test that requires pip install pytest-timeout

why is it required?

If you are just changing the order of the check, we probably don't need a test for it. Usually you want to test your changes but there's no need to test the call order. And if there was, you could do it by mocking the called functions and checking the order.

pytorch_lightning/tuner/lr_finder.py

noamzilo · 2021-01-25T01:05:11Z

test that requires pip install pytest-timeout

why is it required?

If you are just changing the order of the check, we probably don't need a test for it. Usually you want to test your changes but there's no need to test the call order. And if there was, you could do it by mocking the called functions and checking the order.

actually the call order that wasn't tested is what is PR is about. I could just delete the test... should I do that? I think this tests that the user experience is such that no waiting for errors can occur, not just that the calls happen in order.

About the code comment - will commit the str return soon.

tchaton

LGTM !

tests/trainer/test_lr_finder.py

pytorch_lightning/tuner/lr_finder.py

tests/trainer/test_lr_finder.py

tests/trainer/flags/test_fast_dev_run.py

pytorch_lightning/tuner/lr_finder.py

.

codecov · 2021-01-30T10:31:53Z

Codecov Report

Merging #5638 (042bfda) into master (d71659b) will decrease coverage by 0%.
The diff coverage is 92%.

@@           Coverage Diff           @@
##           master   #5638    +/-   ##
=======================================
- Coverage      93%     93%    -0%     
=======================================
  Files         134     134            
  Lines       10056    9919   -137     
=======================================
- Hits         9399    9241   -158     
- Misses        657     678    +21

Co-authored-by: Rohit Gupta <[email protected]>

pytorch_lightning/tuner/lr_finder.py

tests/trainer/test_lr_finder.py

@carmocca

* started to write failing test. just getting into the framework... * started to write failing test. just getting into the framework... * added failing test for misconfiguration of lr finder * made test startup quickly. making sure without the fix it also fails slowly * improved test * fixed for linter * fixed for linter * yet another fix for the linter * yet another fix for the linter * fixed comment by @carmocca * fixed comment by @carmocca * Fix test * chlog * Apply suggestions from code review * Fix test * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update tests/trainer/test_lr_finder.py * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py * Update tests/trainer/test_lr_finder.py Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Rohit Gupta <[email protected]>

@carmocca

* started to write failing test. just getting into the framework... * started to write failing test. just getting into the framework... * added failing test for misconfiguration of lr finder * made test startup quickly. making sure without the fix it also fails slowly * improved test * fixed for linter * fixed for linter * yet another fix for the linter * yet another fix for the linter * fixed comment by @carmocca * fixed comment by @carmocca * Fix test * chlog * Apply suggestions from code review * Fix test * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update tests/trainer/test_lr_finder.py * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py * Update tests/trainer/test_lr_finder.py Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Rohit Gupta <[email protected]>

@carmocca

* started to write failing test. just getting into the framework... * started to write failing test. just getting into the framework... * added failing test for misconfiguration of lr finder * made test startup quickly. making sure without the fix it also fails slowly * improved test * fixed for linter * fixed for linter * yet another fix for the linter * yet another fix for the linter * fixed comment by @carmocca * fixed comment by @carmocca * Fix test * chlog * Apply suggestions from code review * Fix test * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update tests/trainer/test_lr_finder.py * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py Co-authored-by: Rohit Gupta <[email protected]> * Update pytorch_lightning/tuner/lr_finder.py * Update tests/trainer/test_lr_finder.py Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Rohit Gupta <[email protected]>

noamzilo added 5 commits January 15, 2021 18:32

started to write failing test. just getting into the framework...

7952b79

started to write failing test. just getting into the framework...

7e2fa62

added failing test for misconfiguration of lr finder

edc9ed9

made test startup quickly. making sure without the fix it also fails …

d24b797

…slowly

improved test

b51a754

noamzilo requested review from awaelchli, Borda, justusschock, SeanNaren, tchaton and williamFalcon as code owners January 24, 2021 14:47

noamzilo added 6 commits January 24, 2021 16:48

Merge branch 'master' into bugfix/5487_auto_lr_ordering

0ca79c8

fixed for linter

c34042c

Merge branch 'bugfix/5487_auto_lr_ordering' of https://github.com/noa…

0ccefb2

…mzilo/pytorch-lightning into bugfix/5487_auto_lr_ordering

fixed for linter

e7a6c36

yet another fix for the linter

874178c

yet another fix for the linter

404bc9c

noamzilo mentioned this pull request Jan 24, 2021

auto learning rate finder fails AFTER optimization due to misconfiguration #5487

Closed

carmocca reviewed Jan 24, 2021

View reviewed changes

pytorch_lightning/tuner/lr_finder.py Outdated Show resolved Hide resolved

noamzilo added 2 commits January 25, 2021 02:58

Merge branch 'master' into bugfix/5487_auto_lr_ordering

7f1bae7

fixed comment by @carmocca

c6dedc8

fixed comment by @carmocca

03756f2

Borda added the bug Something isn't working label Jan 25, 2021

Borda added this to the 1.1.x milestone Jan 25, 2021

Borda assigned carmocca Jan 26, 2021

Borda requested a review from carmocca January 26, 2021 23:40

mergify bot and others added 2 commits January 29, 2021 13:08

Merge branch 'master' into bugfix/5487_auto_lr_ordering

3bc6ae1

Fix test

fa7255e

tchaton approved these changes Jan 29, 2021

View reviewed changes

Merge branch 'master' into bugfix/5487_auto_lr_ordering

ecbc80d

rohitgr7 previously approved these changes Jan 29, 2021

View reviewed changes

rohitgr7 disabled auto-merge January 29, 2021 18:27

Merge branch 'master' into bugfix/5487_auto_lr_ordering

bc6055c

mergify bot and others added 7 commits January 31, 2021 13:18

Merge branch 'master' into bugfix/5487_auto_lr_ordering

8d01dd3

Merge branch 'master' into bugfix/5487_auto_lr_ordering

7ee4056

Update pytorch_lightning/tuner/lr_finder.py

15ec3cd

Co-authored-by: Rohit Gupta <[email protected]>

Update pytorch_lightning/tuner/lr_finder.py

1008706

Co-authored-by: Rohit Gupta <[email protected]>

Update tests/trainer/test_lr_finder.py

030d6e8

Update pytorch_lightning/tuner/lr_finder.py

70359a3

Co-authored-by: Rohit Gupta <[email protected]>

Update pytorch_lightning/tuner/lr_finder.py

99f09f7

Co-authored-by: Rohit Gupta <[email protected]>

carmocca requested a review from rohitgr7 February 1, 2021 00:58

Merge branch 'master' into bugfix/5487_auto_lr_ordering

cff6bc6

carmocca reviewed Feb 1, 2021

View reviewed changes

pytorch_lightning/tuner/lr_finder.py Outdated Show resolved Hide resolved

Update pytorch_lightning/tuner/lr_finder.py

035b72f

carmocca reviewed Feb 1, 2021

View reviewed changes

tests/trainer/test_lr_finder.py Outdated Show resolved Hide resolved

carmocca and others added 2 commits February 1, 2021 12:52

Update tests/trainer/test_lr_finder.py

71b36d2

Merge branch 'master' into bugfix/5487_auto_lr_ordering

042bfda

rohitgr7 approved these changes Feb 1, 2021

View reviewed changes

rohitgr7 enabled auto-merge (squash) February 1, 2021 13:52

rohitgr7 merged commit 9174fab into Lightning-AI:master Feb 1, 2021

rohitgr7 mentioned this pull request Feb 15, 2021

update lr_finder to check for attribute if not running fast_dev_run #5990

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/5487 auto lr ordering #5638

Bugfix/5487 auto lr ordering #5638

noamzilo commented Jan 24, 2021 •

edited by carmocca

Loading

pep8speaks commented Jan 24, 2021 •

edited

Loading

noamzilo commented Jan 24, 2021

carmocca commented Jan 24, 2021

noamzilo commented Jan 25, 2021 •

edited

Loading

tchaton left a comment

codecov bot commented Jan 30, 2021 •

edited

Loading

Bugfix/5487 auto lr ordering #5638

Bugfix/5487 auto lr ordering #5638

Conversation

noamzilo commented Jan 24, 2021 • edited by carmocca Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Jan 24, 2021 • edited Loading

Comment last updated at 2021-02-01 13:09:50 UTC

noamzilo commented Jan 24, 2021

carmocca commented Jan 24, 2021

noamzilo commented Jan 25, 2021 • edited Loading

tchaton left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2021 • edited Loading

Codecov Report

noamzilo commented Jan 24, 2021 •

edited by carmocca

Loading

pep8speaks commented Jan 24, 2021 •

edited

Loading

noamzilo commented Jan 25, 2021 •

edited

Loading

codecov bot commented Jan 30, 2021 •

edited

Loading