Skip to content

Centralize Async TP Enablement with maybe_enable_async_tp API #1619

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fegin
Copy link
Contributor

@fegin fegin commented Aug 21, 2025

This PR addresses duplicated code related to enabling async TP across different parts of the codebase. It introduces a new API, maybe_enable_async_tp(), which centralizes the enablement logic and is reused consistently in all models.

Note that while this PR fixes one async TP bug in TorchTitan, it does not fully resolve #1613, as there appear to be additional bugs in PyTorch's async TP implementation.

This PR addresses duplicated code related to enabling async TP across different parts of the codebase. It introduces a new API, `maybe_enable_async_tp()`, which centralizes the enablement logic and is reused consistently in all models.

Note that while this PR fixes one async TP bug in TorchTitan, it does
not fully resolve #1613, as
there appear to be additional bugs in PyTorch's async TP implementation.
@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 21, 2025
Copy link
Contributor

@tianyu-l tianyu-l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, had a suggestion.

@@ -139,12 +133,26 @@ def parallelize_llama(
return model


def maybe_enable_async_tp(job_config: JobConfig, tp_mesh: DeviceMesh):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest we take this chance to put it into model-agnostic file. Specifically I'm thinking of torchtitan/distributed/tensor_parallel.py where we can also put NoParallel (https://github.com/pytorch/torchtitan/blob/main/torchtitan/distributed/expert_parallel.py#L116) to.

I'm also thinking we may want to put most apply_ac (and maybe apply_compile) logic to that folder, as they are pretty much the same across all models.

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.

[DeepSeek V3] async TP not effective
2 participants