-
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
3/n Simplify spawn plugins: Merge pre_dispatch
and setup
logic
#11137
Conversation
Note: this will be a breaking change, I saw few use cases user overrides pre_dispatch |
On naming, users might be confused with the strategy's Do you think there's another name which captures what's being done in the strategy class here? |
@ananthsub that's a really good point. We have setup_distributed and setup_envirment to be the |
pre_dispatch
and setup
logic pre_dispatch
and setup
logic
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 !
What does this PR do?
Part of #10987
Merge after #11149
The concept of "dispatch" no longer exists after #10896. This clean up merges two TTP setup calls that no longer need to be separated. The only minor inconvenience is that the
on_fit_start
call is inbetween the two, but on another note after #10896 theon_fit_start
hook is no longer constraint to be exactly at this place.Before:
After:
Follow-up work will include:
Does your PR introduce any breaking changes? If yes, please list them.
Yes, anyone who implemented
pre_dispatch()
will have to move their code tosetup()
.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 🙃
Part of #1 (it's a lie, this is just here to avoid noisy GitHub bot)
cc @tchaton @justusschock @awaelchli @Borda