-
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
Add should_rank_save_checkpoint
property to Training Plugins
#7684
Add should_rank_save_checkpoint
property to Training Plugins
#7684
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7684 +/- ##
=======================================
- Coverage 93% 88% -5%
=======================================
Files 199 199
Lines 12957 12960 +3
=======================================
- Hits 11991 11371 -620
- Misses 966 1589 +623 |
Hello @kaushikb11! Thanks for updating this PR.
Comment last updated at 2021-05-24 21:11:39 UTC |
pytorch_lightning/plugins/training_type/training_type_plugin.py
Outdated
Show resolved
Hide resolved
thanks for following up on this @kaushikb11 ! I requested changes because of possible inconsistencies that emerge from |
should_save_checkpoint
property to Training Pluginsshould_rank_save_checkpoint
property to Training Plugins
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.
@SeanNaren @shuyingsunshine21 i think this will also help with FSDP saving sharded state
@property | ||
def should_rank_save_checkpoint(self) -> bool: | ||
return self.accelerator.training_type_plugin.should_rank_save_checkpoint | ||
|
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.
Are you sure it's okay adding a convenience function in the trainer for it?
Do you expect/allow users to use/rely on it directly?
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.
Yeah, I don't expect it to be used by users, but for internal use.
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.
My point is that if it is for internal use, the property should be prepended by _
.
It's ugly accessing an underscore attribute but we need a mechanism to avoid having to deprecate later.
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?
Fixes #7678
Related to link
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?
Make sure you had fun coding 🙃