-
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
Introduce Strategy
in favour of TrainingTypePlugin
#10548
Conversation
I'd drop
and over time, these classes could be renamed like @four4fish what do you think? |
Yeah, agree, I think we should drop plugin this word completely. Also remove the |
rank_zero_deprecation( | ||
"`pytorch_lightning.plugins.training_type.ddp2.DDP2Plugin` has been deprecated in v1.6" | ||
" and will be removed in v1.8. Use `pytorch_lightning.plugins.strategy.ddp2.DDP2Plugin`" | ||
" instead." | ||
) |
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.
Since you are making these changes to each file, git does not properly move the file and interprets it as a deletion + addition.
I strongly suggest we do this in two PRs:
- Move the code.
- Add the backwards compatible paths.
This is to preserve the git history.
In between, the backwards compatibility would be broken but should be okay as there will not be a release in-between.
Since the Meta team relies on master, we just need them to skip the commits including step (1) until step (2) is merged when they update their internal fork. Would that be okay @ananthsub?
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.
And move files lost the historical tracks, it's hard to tracking back changes and old issues related to it. Is moving files and name change necessary?
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.
git mv
supports moving the location and changing the filename.
The problem is then adding changes to the file.
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.
git mv
supports moving the location and changing the filename.The problem is then adding changes to the file.
Got you! that's why you suggested separate the steps, make sense! thanks~~
StrateyPlugin
in favour of TrainingTypePlugin
StrategyPlugin
in favour of TrainingTypePlugin
StrategyPlugin
in favour of TrainingTypePlugin
Strategy
in favour of TrainingTypePlugin
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions. |
This pull request is going to be closed. Please feel free to reopen it create a new from the actual master. |
What does this PR do?
Part of #9932
As this PR is already huge enough, breaking the task into these follow up PRs:
_training_type_plugin
in favour of_strategy_plugin
strategy_plugin
argument toAccelerator
Does your PR introduce any breaking changes? If yes, please list them.
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 🙃