Skip to content

Add sp_min_token_num=0 to E2E correctness tests for SP and AsyncTP - #38151

Closed
ProExpertProg with Copilot wants to merge 1 commit into
mainfrom
copilot/add-sp-min-token-to-e2e-tests
Closed

ProExpertProg with Copilot wants to merge 1 commit into
mainfrom
copilot/add-sp-min-token-to-e2e-tests

Conversation

Copilot AI commented Mar 25, 2026 •

Copy link
Copy Markdown
Contributor

When sp_min_token_num is unset, vLLM auto-computes an SP threshold via get_sequence_parallelism_threshold(). For small models (e.g. hmellor/tiny-random-LlamaForCausalLM) or dummy-loaded models with overridden hidden_size, this returns None (below SP_MIN_HIDDEN_SIZE), silently disabling SP — meaning the correctness tests weren't actually exercising SP or AsyncTP paths.

Changes

  • test_sequence_parallel.py: Add "sp_min_token_num": 0 to pass_config in _compare_sp() to force SP enabled regardless of model size.
  • test_async_tp.py: Add "sp_min_token_num": 0 to pass_config in test_async_tp_pass_correctness() — AsyncTP internally requires SP, same silent-disable risk applies.

Consistent with the existing pattern in tests/compile/fusions_e2e/test_tp2_async_tp.py which already sets sp_min_token_num=512 for the same reason. Using 0 is more aggressive: SP applies at any batch size.

compilation_config = {
    "mode": CompilationMode.VLLM_COMPILE,
    "compile_sizes": [4, 8],
    "pass_config": {
        "enable_sp": True,
        "fuse_gemm_comms": fuse_gemm_comms,
        # Override threshold to ensure SP is applied even for small models.
        "sp_min_token_num": 0,
    },
}

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

Copilot AI changed the title [WIP] Add CompilationConfig.pass_config.enable_sp_min_token to E2E tests Add sp_min_token_num=0 to E2E correctness tests for SP and AsyncTP Mar 25, 2026
Copilot AI requested a review from ProExpertProg March 25, 2026 21:54
Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
Agent-Logs-Url: https://github.com/vllm-project/vllm/sessions/f8e4f0ad-7a0f-4253-8a28-7a0100eacae5
@ProExpertProg
ProExpertProg force-pushed the copilot/add-sp-min-token-to-e2e-tests branch from 6cf77e4 to b65206a Compare April 29, 2026 18:11
@ProExpertProg
ProExpertProg requested a review from zou3519 April 29, 2026 18:11
@ProExpertProg ProExpertProg added the ready ONLY add when PR is ready to merge/full CI is needed label Apr 29, 2026
@mergify

mergify Bot commented May 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, @Copilot.

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

needs-rebase ready ONLY add when PR is ready to merge/full CI is needed torch.compile

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants