fix infinite recovery #217
Merged
Conversation
This file contains hidden or 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
This was referenced Jun 16, 2025
Merged
facebook-github-bot
added
the
CLA Signed
tushar00jain
force-pushed
the
pr217
branch
2 times, most recently
from
0bdb164 to
5c31448
Compare
d4l3k
approved these changes
Jun 17, 2025
H-Huang
approved these changes
Jun 17, 2025
tushar00jain
force-pushed
the
pr217
branch
6 times, most recently
from
fad3f3e to
d61f4f4
Compare
d4l3k
approved these changes
Jun 18, 2025
torchft/local_sgd.py
Outdated
| @@ -559,7 +592,7 @@ def __init__( | |||
| _StreamingDiLoCoFragment( | |||
| manager, | |||
| model_fragment, | |||
| math.floor((sync_every / len(model_fragments)) * (i + 1)), | |||
| (sync_every // len(model_fragments) * (i + 1)), | |||
There was a problem hiding this comment.
Should we assert that this is an exact multiple of model_fragments? I suppose it doesn't matter too much if it's not exact but might surprise people?
There was a problem hiding this comment.
i feel more asserts will require people to remember more things on how to tune the settings? don't have a strong pref if you think that's easier to reason about though, happy to make changes later as well
H-Huang
reviewed
Jun 18, 2025
| fragment.prepare_sync() | ||
|
|
||
| for i, fragment in enumerate(self._fragments): | ||
| if not fragment.should_sync_fragment(step): | ||
| continue | ||
|
|
||
| if i not in self._first_prepare_sent: |
There was a problem hiding this comment.
can you add a comment when we would get into this case? It looks like we set it in the loop before so i'm confused.
There was a problem hiding this comment.
the loop before could run for a different fragment (not the one we're syncing) depending on the sync schedule
Summary: - we don't increase the max_step when a node is catching up because we don't call should_commit - this can lead the node always being behind and get into an infinite recovery loop - note, this can result in the global parameters falling out of sync, the diff includes an RFC on how to fix that if we need to - document another case where `should_commit` can return `True` but it shouldn't because allreduce failed (this is also relvant only to the case when we can have pending inflight allreduce) - make an assert based on the fragment sync schedule to make sure we don't run into this Test Plan: - tested on a cluster of 3 nodes by removing and adding a node - the `max_step` and `local_step` increase in the manager's state dict after both failure and recovery metrics from the healthy node <img width="1103" alt="Screenshot 2025-06-15 at 10 53 28 PM copy" src="https://github.com/user-attachments/assets/8640780c-fd20-4266-aa3c-3116776a9c68" /> metrics from the failed and recovered node <img width="1101" alt="Screenshot 2025-06-15 at 10 56 49 PM copy" src="https://github.com/user-attachments/assets/cc2a1c57-715f-4e0a-8e00-7c62da525dc3" />
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.
Summary:
should_commitcan returnTruebut it shouldn't because allreduce failed (this is also relvant only to the case when we can have pending inflight allreduce)Test Plan:
max_stepandlocal_stepincrease in the manager's state dict after both failure and recoverymetrics from the healthy node
metrics from the failed and recovered node
Stack created with Sapling. Best reviewed with ReviewStack.