Fix/mismatched toggle optimizer #7563

Lucklyric · 2021-05-15T20:19:11Z

What does this PR do?

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?
NA 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-05-15T20:20:34Z

Codecov Report

Merging #7563 (4f4f75e) into master (20f6337) will decrease coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #7563    +/-   ##
=======================================
- Coverage      92%     88%    -4%     
=======================================
  Files         197     198     +1     
  Lines       12878   12835    -43     
=======================================
- Hits        11899   11289   -610     
- Misses        979    1546   +567

carmocca

Can you resolve conflicts and add a test?

CHANGELOG.md

Refs Lightning-AI#7405 chore: update CHANGELOG [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix: resolve a confict chore: update changelog

Lucklyric · 2021-05-19T06:06:16Z

Dear @carmocca , I just resolved the conflict. Because I only changed the place of untoggle_optimizer, those codes seem to be still covered by existing tests.

carmocca · 2021-05-19T22:10:33Z

I just resolved the conflict

Thanks!

those codes seem to be still covered by existing tests.

Coverage is not enough. Needs a test to ensure the new behaviour is correct and to avoid future regressions

tchaton · 2021-05-21T11:48:41Z

Dear @Lucklyric,

Mind making a test triggering the grad_fn error on master and working there.

Best,
T.C

Lucklyric · 2021-05-21T15:34:50Z

Dear @Lucklyric,

Mind making a test triggering the grad_fn error on master and working there.

Best,
T.C

Will do that. Thank you!

pep8speaks · 2021-05-22T15:44:11Z

Hello @Lucklyric! Thanks for updating this PR.

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

Comment last updated at 2021-05-22 18:24:33 UTC

for more information, see https://pre-commit.ci

Lucklyric · 2021-05-22T16:16:36Z

Dear @carmocca @tchaton , added a test that fails in master and works here. I'm not sure if this is a proper test, so please feel free to provide feedback. Thanks

tchaton · 2021-05-22T17:31:01Z

Hey @Lucklyric @carmoccam confirm this test is failing on master.
Great job there !

tchaton

LGTM !

tests/trainer/optimization/test_multiple_optimizers.py

Co-authored-by: ananthsub <[email protected]>

carmocca

Some nits. Thanks!

tests/trainer/optimization/test_multiple_optimizers.py

Co-authored-by: Carlos Mocholí <[email protected]>

…word args

Lucklyric · 2021-05-22T19:04:24Z

Some nits. Thanks!

Thanks for your comments, @carmocca . Updated!

* fix: avoid potential mismatched toggling of optimzier Refs #7405 chore: update CHANGELOG [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix: resolve a confict chore: update changelog * feat: add a test that fails in master * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix typo in tests/trainer/optimization/test_multiple_optimizers.py Co-authored-by: ananthsub <[email protected]> * Polish tests/trainer/optimization/test_multiple_optimizers.py Co-authored-by: Carlos Mocholí <[email protected]> * Polish tests/trainer/optimization/test_multiple_optimizers.py Co-authored-by: Carlos Mocholí <[email protected]> * fix: change placeholder in optimizer_step from positional args to keyword args Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: ananthsub <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]> fix whitespace fix parentheses

Lucklyric marked this pull request as ready for review May 15, 2021 20:51

Lucklyric requested review from awaelchli, Borda, carmocca, justusschock, kaushikb11, SeanNaren, tchaton and williamFalcon as code owners May 15, 2021 20:51

mergify bot added the has conflicts label May 17, 2021

carmocca suggested changes May 17, 2021

View reviewed changes

SkafteNicki reviewed May 18, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Lucklyric force-pushed the fix/mismatched_toggle_optimizer branch from a4fbea3 to 42b931d Compare May 19, 2021 04:08

mergify bot removed the has conflicts label May 19, 2021

fix: avoid potential mismatched toggling of optimzier

032cdad

Refs Lightning-AI#7405 chore: update CHANGELOG [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix: resolve a confict chore: update changelog

Lucklyric force-pushed the fix/mismatched_toggle_optimizer branch from 42b931d to 032cdad Compare May 19, 2021 04:11

feat: add a test that fails in master

2a488a1

[pre-commit.ci] auto fixes from pre-commit.com hooks

d1c802e

for more information, see https://pre-commit.ci

tchaton approved these changes May 22, 2021

View reviewed changes

ananthsub reviewed May 22, 2021

View reviewed changes

tests/trainer/optimization/test_multiple_optimizers.py Outdated Show resolved Hide resolved

fix typo in tests/trainer/optimization/test_multiple_optimizers.py

41153cd

Co-authored-by: ananthsub <[email protected]>

ananthsub approved these changes May 22, 2021

View reviewed changes

carmocca approved these changes May 22, 2021

View reviewed changes

tests/trainer/optimization/test_multiple_optimizers.py Outdated Show resolved Hide resolved

tests/trainer/optimization/test_multiple_optimizers.py Outdated Show resolved Hide resolved

carmocca added this to the v1.3.x milestone May 22, 2021

carmocca added the bug Something isn't working label May 22, 2021

Lucklyric and others added 3 commits May 22, 2021 12:01

Polish tests/trainer/optimization/test_multiple_optimizers.py

d4b5b14

Co-authored-by: Carlos Mocholí <[email protected]>

Polish tests/trainer/optimization/test_multiple_optimizers.py

74dc60f

Co-authored-by: Carlos Mocholí <[email protected]>

fix: change placeholder in optimizer_step from positional args to key…

4f4f75e

…word args

carmocca merged commit 01109cd into Lightning-AI:master May 23, 2021

awaelchli added a commit that referenced this pull request May 25, 2021

integrate #7563

ec9da3b

Borda mentioned this pull request May 25, 2021

1.3.x cherries 🍒 #7467

Closed

Borda mentioned this pull request May 26, 2021

Weekly Patch Release v1.3.3 [full merge, no squash] #7712

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/mismatched toggle optimizer #7563

Fix/mismatched toggle optimizer #7563

Lucklyric commented May 15, 2021 •

edited

Loading

codecov bot commented May 15, 2021 •

edited

Loading

carmocca left a comment

Lucklyric commented May 19, 2021 •

edited

Loading

carmocca commented May 19, 2021

tchaton commented May 21, 2021

Lucklyric commented May 21, 2021

pep8speaks commented May 22, 2021 •

edited

Loading

Lucklyric commented May 22, 2021

tchaton commented May 22, 2021

tchaton left a comment

carmocca left a comment

Lucklyric commented May 22, 2021

Fix/mismatched toggle optimizer #7563

Fix/mismatched toggle optimizer #7563

Conversation

Lucklyric commented May 15, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented May 15, 2021 • edited Loading

Codecov Report

carmocca left a comment

Choose a reason for hiding this comment

Lucklyric commented May 19, 2021 • edited Loading

carmocca commented May 19, 2021

tchaton commented May 21, 2021

Lucklyric commented May 21, 2021

pep8speaks commented May 22, 2021 • edited Loading

Comment last updated at 2021-05-22 18:24:33 UTC

Lucklyric commented May 22, 2021

tchaton commented May 22, 2021

tchaton left a comment

Choose a reason for hiding this comment

carmocca left a comment

Choose a reason for hiding this comment

Lucklyric commented May 22, 2021

Lucklyric commented May 15, 2021 •

edited

Loading

codecov bot commented May 15, 2021 •

edited

Loading

Lucklyric commented May 19, 2021 •

edited

Loading

pep8speaks commented May 22, 2021 •

edited

Loading