-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Move the lightning_optimizers
ownership to the Strategy
#11444
Conversation
lightning_optimizers
ownership to the Strategy
lightning_optimizers
ownership to the Strategy
apart from the comments here, there is no issue or description. for others it will be hard to get the picture of what you briefly explained offline. would you mind adding that outline somewhere? |
Updated the top post @awaelchli. If you think this PR needs further discussion, I can also open an issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
Codecov Report
@@ Coverage Diff @@
## master #11444 +/- ##
=======================================
- Coverage 88% 88% -0%
=======================================
Files 194 194
Lines 17005 17000 -5
=======================================
- Hits 15002 14994 -8
- Misses 2003 2006 +3 |
What does this PR do?
Context
Currently, the
Strategy
is the object holding the optimizers. We have an internal wrapper called theLightningOptimizer
which maps 1:1 to eachOptimizer
.The list of lightning optimizers is currently owned by the
Trainer
. There's no requirement for that anymore so this PR moves the ownership to theStrategy
, together with the optimizer references.Benefits
Strategy.optimizers
list gets set, thestrategy._lightning_optimzers
dictionary des too, this avoids stale references.Trainer
LightningOptimizer
and the_LiteOptimizer
in a follow-up: Unify theLightningOptimizer
andLiteOptimzier
#11452trainer.lightning_optimizers
, as there's no reason why users would need to access or know about this internal wrapper.Does your PR introduce any breaking changes? If yes, please list them.
None
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
cc @Borda @justusschock @awaelchli @akihironitta @rohitgr7 @tchaton