Skip to content

fix compute/communication overlap for gloo #240

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

Merged
merged 2 commits into from
Aug 2, 2025

Conversation

tushar00jain
Copy link
Contributor

@tushar00jain tushar00jain commented Jul 22, 2025

Summary:

  • we current wait for pg work's future when preparing for a fragment
  • if we use gloo, this blocks the cpu
  • move the wait call to when we perform the actual sync of the fragment

Stack created with Sapling. Best reviewed with ReviewStack.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 22, 2025
@tushar00jain tushar00jain marked this pull request as draft July 22, 2025 22:11
@tushar00jain tushar00jain force-pushed the pr240 branch 4 times, most recently from dbec11e to 5228ee8 Compare July 24, 2025 21:53
@tushar00jain tushar00jain changed the title use block_current_stream work api wait for futures while syncing fragments Jul 24, 2025
@tushar00jain tushar00jain changed the title wait for futures while syncing fragments use block_current_stream work api Jul 24, 2025
@tushar00jain tushar00jain changed the title use block_current_stream work api wait for futures while syncing fragments Jul 24, 2025
@tushar00jain tushar00jain force-pushed the pr240 branch 14 times, most recently from c93ad11 to bfb92ff Compare July 25, 2025 21:20
@tushar00jain tushar00jain marked this pull request as ready for review July 25, 2025 21:21
@tushar00jain tushar00jain requested a review from d4l3k July 25, 2025 21:21
Summary:
- we currently do some validation on the training in the regression test
- the force recovery on first step interferes with this because it makes the test non determinstic, particularly because after the recovery, replica takes non deterministic number of steps that makes the gradients non determinstic
- to fix this, perform a quorum inside fake training loop for the regression test before doing any training
- we also need to increase manager step count by 2, so we do 2 should_commit, because we have 2 fragments and we're testing numerics as if we started from step 0 -- starting from step 2 gives us the same sync schedule for fragments as starting from step 0
Summary:
- we current wait for pg work's future when preparing for a fragment
- if we use gloo, this blocks the cpu
- move the wait call to when we perform the actual sync of the fragment
@tushar00jain tushar00jain merged commit 22b8fa1 into pytorch:main Aug 2, 2025
14 checks passed
@tushar00jain tushar00jain deleted the pr240 branch August 2, 2025 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants