-
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
Decouple Tuner from Trainer #16462
Decouple Tuner from Trainer #16462
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
Phew! Nice work
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.
I love the simplification, just thinking about the new name...
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.
Implementation looks good. Just a general question:
Do we really need a new tuner object or can these be callbacks only?
The tuner object is not new, it was always a public class (maybe it should not have been from the start). The callbacks were introduced recently yes, but there are features in the tuner that are a bit unclear how you would use them with just callbacks. For example, an interaction like this: tuner = Tuner()
finder = tuner.lr_find(...)
finder.plot()
finder.suggestion() etc. The good thing is, after this PR, the development on the tuner code can happen without impacting anything on the Trainer side. So there is still the possibility to go in either direction. |
What does this PR do?
Removes:
New usage:
Furthermore, the same tuner features are available through the
LearningRateFinder
andBatchSizeFinder
callbacks.Note: The tuning callbacks still rely on the Trainer handling the
TunerExitException
. Haven't yet found a way to decouple that. This can be discussed in a follow-up.Fixes #5374
Fixes #9103
Does your PR introduce any breaking changes? If yes, please list them.
Yes, justified by the 2.0 initiative of making Trainer leaner.
Before submitting
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:
Did you have fun?
I made sure I had fun coding 🙃
cc @Borda @justusschock