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.

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

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

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

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

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

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

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

@pep8speaks
Copy link

pep8speaks commented Oct 16, 2020

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
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
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
@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
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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
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