Hotfix: load_state_from_peers with offload_optimizer #417
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #417 +/- ##
==========================================
- Coverage 83.68% 83.65% -0.04%
==========================================
Files 77 77
Lines 7785 7788 +3
==========================================
Hits 6515 6515
- Misses 1270 1273 +3
|
justheuristic
changed the title
borzunov
reviewed
Dec 2, 2021
| @@ -631,7 +631,8 @@ def load_state_from_peers(self, **kwargs): | |||
| Attempt to download the latest optimizer state from peers and update trainer parameters/statistics. | |||
| :returns: whether or the averager succeeded in loading parameters | |||
| """ | |||
| main_parameters_and_extras = tuple(chain(self.main_parameters, self.extra_tensors)) | |||
| opt_parameters = tuple(param for param_group in self.optimizer.param_groups for param in param_group["params"]) | |||
| main_parameters_and_extras = tuple(chain(opt_parameters, self.extra_tensors)) | |||
There was a problem hiding this comment.
What does it mean? If self.main_parameters are invalid at this stage, shouldn't we replace them?
There was a problem hiding this comment.
If offload_optimizer == False, opt_parameters ARE main parameters
If offload_optimizer == True, we update main parameters in L665
borzunov
approved these changes
Dec 2, 2021
|
@SeanNaren JFYI: I see that you've reacted to this, but just in case, you will need to switch to newer hivemind version after this gets merged :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
hivemind.optim.experimental.Optimizer with offload_optimizer=True behaved incorrectly when loading state from peers.
It would load the state into local parameters, and then it was meant to write new parameters into the offloaded optimizer, but actually overriden the newly loaded parameters with old offloaded ones. The PR fixes this.
Demonstration 1: peers with uneven performance are now handled correctly (same as in CollaborativeOptimizer)
Demonstration 2: peers that join late are now handled correctly even with offload_optimizer=True. In the demo below, the orange peer started late.
Before fix, late peers with offload_optimizer would have correct main parameters, but their optimizer would actually have wrong parameters
