-
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
Refactor Strategy._move_optimizer_states
as utility functions
#11758
Refactor Strategy._move_optimizer_states
as utility functions
#11758
Conversation
@ananthsub do you think we could unify this with some similar lines of code we have in |
Yes exactly. that was my motivation for this PR after seeing the bug here: #11741 See my comment here: #11757 (comment) |
c524b25
to
dd79460
Compare
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 !
6fdd832
to
3bd704f
Compare
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: thomas chaton <thomas@grid.ai>
What does this PR do?
Refactoring to make the code easier to test by splitting this out as a separate function. I didn't see any existing tests for this. This will simplify #11757
Does your PR introduce any breaking changes? If yes, please list them.
Yes, if custom Strategy implementations overrode
_move_optimizer_to_device
, they will break after this PRBefore 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 🙃