-
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
Deprecate Trainer.should_rank_save_checkpoint
property
#11068
Deprecate Trainer.should_rank_save_checkpoint
property
#11068
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## master #11068 +/- ##
========================================
- Coverage 92% 88% -4%
========================================
Files 177 177
Lines 16492 16502 +10
========================================
- Hits 15151 14551 -600
- Misses 1341 1951 +610 |
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
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.
I don't understand how we can deprecate this on the trainer while at the same time remove it on the strategy. You will have to copy this implementation into the deprecated property method:
return isinstance(self.ttp, TPUSpawn) and self.local_rank == 0 or self.is_global_zero
and it won't cover custom plugins.
So yeah, this will be confusing.
What does this PR do?
see context: #11035 (comment)
original PR: #9433
original issue: #9074
Go through deprecation for public property
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 🙃