Skip to content
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

process_group: register via public API #14

Merged
merged 1 commit into from
Nov 22, 2024
Merged

process_group: register via public API #14

merged 1 commit into from
Nov 22, 2024

Conversation

d4l3k
Copy link
Member

@d4l3k d4l3k commented Nov 22, 2024

This registers the ProcessGroups via the public APIs (Backend.register_backend and dist.new_group) rather than modifying internal state.

This makes things a bit messier but fits better into the usual PyTorch init logic. This requires a global PG to be initialized. A future PR could detect if a PG is initialized and register this as the global PG if necessary.

Weirdly -- with how this works it ends up returning the exact same ProcessGroup object from dist.new_group. I'm not quite sure how that actually works. The Backend wrapper seems to unwrap itself when casted to PG.

Test plan:

pytest torchft/process_group_test.py

@d4l3k d4l3k requested a review from fegin November 22, 2024 06:31
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 22, 2024
@fegin
Copy link
Contributor

fegin commented Nov 22, 2024

I like this approach even though it requires the global PG.

@fegin fegin merged commit ee864cf into main Nov 22, 2024
4 checks passed
@fegin fegin deleted the d4l3k/pg_backend branch November 22, 2024 22:54
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