Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimizer closure #4190

Merged
merged 20 commits into from
Oct 21, 2020
Merged

Optimizer closure #4190

merged 20 commits into from
Oct 21, 2020

Conversation

justusschock
Copy link
Member

@justusschock justusschock commented Oct 16, 2020 •
edited by Borda
Loading

What does this PR do?

Fixes #4188 and also fixes #2785

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@justusschock justusschock added the bug Something isn't working label Oct 16, 2020
@justusschock justusschock self-assigned this Oct 16, 2020
@mergify mergify bot requested a review from a team October 16, 2020 08:41
@rohitgr7
Copy link
Contributor

rohitgr7 commented Oct 16, 2020 •
edited
Loading

Fixes #4083 too right?

@justusschock
Copy link
Member Author

@rohitgr7 Not yet tested, net to get it working first :D

@williamFalcon
Copy link
Contributor

williamFalcon commented Oct 16, 2020 •
edited
Loading

nice. when it's ready, can you add tests that use the mock + count to verify that we perform the expected number of backward passes? also with a non-lbfgs optimizer to make sure we don't break current functionality

@justusschock
Copy link
Member Author

justusschock commented Oct 16, 2020 •
edited
Loading

@williamFalcon I think @awaelchli is currently looking into this.

But no matter who does this: There will definitely be tests!

@justusschock justusschock mentioned this pull request Oct 16, 2020
Closed
@awaelchli awaelchli mentioned this pull request Oct 16, 2020
Closed
@pep8speaks
Copy link

pep8speaks commented Oct 16, 2020 •
edited
Loading

Hello @justusschock! Thanks for updating this PR.

Line 710:121: E501 line too long (122 > 120 characters)

Comment last updated at 2020-10-21 14:34:14 UTC

@edenlightning edenlightning added this to the 1.0.3 milestone Oct 19, 2020
@justusschock justusschock changed the title WIP: Optimizer closure Optimizer closure Oct 21, 2020
@justusschock justusschock mentioned this pull request Oct 21, 2020
Closed
awaelchli
awaelchli reviewed Oct 21, 2020
View reviewed changes
pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team October 21, 2020 13:55
@mergify mergify bot requested a review from a team October 21, 2020 14:18
@mergify mergify bot requested a review from a team October 21, 2020 14:31
awaelchli
awaelchli approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@mergify mergify bot requested a review from a team October 21, 2020 16:44
tchaton
tchaton approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition !

@SeanNaren SeanNaren merged commit 0ec4107 into master Oct 21, 2020
@SeanNaren SeanNaren deleted the optimizer_closure branch October 21, 2020 18:34
@awaelchli
Copy link
Contributor

The GPU tests did not run, because of horovod installation failing, so we did not see the problem with amp closure.

@awaelchli awaelchli mentioned this pull request Oct 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaton tchaton tchaton approved these changes

@SeanNaren SeanNaren SeanNaren approved these changes

@awaelchli awaelchli awaelchli approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@lezwon lezwon Awaiting requested review from lezwon

@rohitgr7 rohitgr7 Awaiting requested review from rohitgr7

Assignees

@justusschock justusschock

Labels
bug Something isn't working
Projects
None yet
Milestone
pl:1.0.x
Development

Successfully merging this pull request may close these issues.

To many backwards with LBFGS Pass second-order closure to all optimizers (not just LBFGS)
9 participants
@justusschock @rohitgr7 @williamFalcon @pep8speaks @awaelchli @lezwon @SeanNaren @tchaton @edenlightning