Skip to content

[WIP] SP+AsyncTP piecewise compilation fix + per-matmul heuristic gating - #37489

Closed
tianrengao wants to merge 1 commit into
vllm-project:mainfrom
tianrengao:tianren/sp-asynctp-piecewise-fix
Closed

tianrengao wants to merge 1 commit into
vllm-project:mainfrom
tianrengao:tianren/sp-asynctp-piecewise-fix

Conversation

@tianrengao

Copy link
Copy Markdown
Contributor

Summary

Fix SP+AsyncTP crash with piecewise compilation and add per-matmul heuristic gating.

Changes

  1. Piecewise crash fix: _remove_mismatched_epilogue_copies() + _fix_cross_submod_outputs() — SP changes tensor shapes at submod boundaries, causing assert_size_stride failures
  2. Per-matmul heuristic: Decision tree from AutoHeuristic gates fusion via extra_check, preventing 2-3x decode regression at small batch sizes
  3. Config: async_tp_min_tokens threshold + enable_sp_and_async_tp() runtime check

Status

⚠️ Draft — SP is currently disabled in upstream (IS_DENSE=False, #25689). These changes are preserved for future work.

Benchmark results show no meaningful serving benefit on single-node NVLink:

  • Llama-2-7b TP=2: 1.01-1.02x (noise)
  • Meta-Llama-3-70B TP=8: 0.98x

See benchmarks/SP_ASYNCTP_BENCHMARK_REPORT.md for details.

Summary of changes:
1. Fix SP+piecewise compilation crash: _remove_mismatched_epilogue_copies()
   and _fix_cross_submod_outputs() handle shape mismatches at submod
   boundaries when SP transforms AllReduce->RMSNorm into
   ReduceScatter->RMSNorm->AllGather.

2. Per-matmul async TP heuristic: Decision tree from AutoHeuristic
   (D176943) gates fusion via extra_check in collective_fusion.py.
   Prevents Inductor always-fuse rule from causing 2-3x decode
   regression at small batch sizes (M<=256).

3. Config: async_tp_min_tokens threshold (default=128) for coarse-grained
   batch-size gating. enable_sp_and_async_tp() runtime check.

Note: SP is currently disabled in upstream vLLM (IS_DENSE=False, see
issue vllm-project#25689). These changes are preserved as a draft for future work.

Benchmark results (no meaningful serving benefit on NVLink):
- Llama-2-7b TP=2: 1.01-1.02x (noise)
- Meta-Llama-3-70B TP=8: 0.98x (slight regression)
See benchmarks/SP_ASYNCTP_BENCHMARK_REPORT.md for details.
@mergify mergify Bot added the performance Performance-related issues label Mar 18, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a fix for a crash when using Sequence Parallelism (SP) with Asynchronous Tensor Parallelism (AsyncTP) in piecewise compilation mode. It also adds a per-matmul heuristic to gate fusions, preventing performance regressions at small batch sizes. The changes include new logic to handle tensor shape modifications at submodule boundaries, a new configuration option async_tp_min_tokens for coarse-grained gating, and new benchmark scripts and tests. My review found one high-severity issue related to error handling in the new heuristic check. Otherwise, the changes appear well-structured and address the described problems effectively.

Comment on lines +49 to +50
except Exception:
return True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The use of a broad except Exception: is risky as it can hide various errors during shape extraction for the heuristic check (e.g., KeyError, AttributeError, IndexError). If an unexpected error occurs, it will be silently caught, and the function will default to returning True, causing the fusion to be applied. This could lead to incorrect behavior or mask underlying bugs in the pattern matching. It would be safer to catch more specific exceptions and log a warning to facilitate debugging.

Suggested change
except Exception:
return True
except (KeyError, IndexError, AttributeError) as e:
logger.warning(
"Failed to extract matmul shapes for async TP heuristic check: %s. "
"Defaulting to allowing fusion.",
e,
exc_info=True,
)
return True

@mergify

mergify Bot commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @tianrengao.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Mar 23, 2026
@mergify mergify Bot removed the needs-rebase label Apr 29, 2026
@mergify

mergify Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @tianrengao.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@WoosukKwon

Copy link
Copy Markdown
Collaborator

Closing as superseded by #60517, which removed the torch.compile-based sequence parallelism pass and AsyncTP (along with enable_sp,
fuse_gemm_comms, sp_min_token_num, and their tests). The code this PR modifies no longer exists. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants