Repository navigation
[Bugfix] Keep the pynccl all-reduce out-of-place by default - #1284
Merged
Merged
Conversation
#1277 made pynccl reduce every all-reduce of 4194304 or more elements in place. The vllm.all_reduce op is registered as out-of-place, so compiled graphs may read its input again after the call; reducing in place hands those readers the summed tensor instead of the rank's own partial. Qwen3.8-27B NVFP4 + DFlash2 on TP4 V100 crosses the threshold on every prefill of about 820 tokens or more. Greedy target output is unchanged (identical tokens on all eight 1K/8K fixtures, identical logprobs at 1K), but the draft context changes: speed-fixture tokens per round fell from 2.875/2.974 to 2.655/2.613 at 1K/8K. Unset VLLM_INPLACE_AR_MIN now means out-of-place, except with VLLM_V41_PACKED_KV=1, where the DeepSeek-V4.1 packed-KV prefill keeps the in-place reduction it was qualified with. An explicit value still wins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep the communicator free of env and model coupling (check-layering): envs.py now resolves the default, so the communicator reads envs.VLLM_INPLACE_AR_MIN. Unset still means out-of-place, except with VLLM_V41_PACKED_KV=1. Also drop the model names that #1265 added to two comments in SpeculativeConfig, which pushed its model coupling over the baseline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#1265 added five lines to vllm/config/speculative.py, which moved the SpeculativeConfig line anchors in runtime_reference.md and left check-architecture-reference failing on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 28, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
#1277 (DeepSeek-V4.1-Flash) made the pynccl path reduce every all-reduce of 4,194,304 or more elements in place, for all models.
vllm.all_reduceis registered as an out-of-place op (direct_register_custom_opwithoutmutates_args, dispatched to_all_reduce_out_place), so compiled graphs may read its input again after the call. Reducing in place hands those readers the summed tensor instead of the rank's own partial.Qwen3.8-27B NVFP4 + DFlash2 (TP4, hidden 5120) crosses the threshold on every prefill of about 820 tokens or more. The target is not affected, but the DFlash2 draft context is, and acceptance drops.
This PR makes an unset
VLLM_INPLACE_AR_MINmean out-of-place, except underVLLM_V41_PACKED_KV=1. There the DeepSeek-V4.1 packed-KV prefill keeps the in-place reduction it was qualified with (its launcherbenchmarks/deepseek_v41/tools/run_serve3.shalready setsVLLM_V41_PACKED_KV=1). An explicitVLLM_INPLACE_AR_MINvalue still wins.vllm/envs.pyresolves the default, so the communicator only readsenvs.VLLM_INPLACE_AR_MINand gains no env or model coupling.The second commit also removes the model name that #1265 added to two comments in
SpeculativeConfig. That name pushedvllm/config/speculative.pyover itscheck-layeringbaseline (142 -> 144).Test Plan
tests/distributed/test_inplace_all_reduce_default.py(new, CPU): unset → 0, packed KV → 4194304, explicit value wins in both modes.tests/test_envs.py(58 passed),tools.generate_env_reference --check,tools/pre_commit/check_env_metadata.py,tools/pre_commit/check_layering.py, ruff 0.14.0 check/format.VLLM_V41_PACKED_KV=14194304;VLLM_V41_PACKED_KV=1 VLLM_INPLACE_AR_MIN=00;VLLM_INPLACE_AR_MIN=83886088388608.Test Result
Speed-fixture tokens per round (1K / 8K input). Each row is deterministic across both repeats:
VLLM_PYNCCL_ALLGATHER=0VLLM_INPLACE_AR_MIN=0Greedy check, main vs
VLLM_INPLACE_AR_MIN=0, 300 tokens on four 1K and four 8K fixtures: identical tokens on all 8. Logprobs are bitwise identical at 1K and differ by at most 0.032 at 8K. So the target output is preserved, and the lost acceptance comes from the draft context.Short official-style prompts (GSM8K / MT-Bench / HumanEval, 16 each, temperature 1.0, thinking on) never reach the threshold. Their acceptance length was unchanged between 8a3c030 and main (5.31/3.73/4.78 vs 5.24/3.81/4.86). The regression only shows up for long prompts.
check-layeringstill reportsvllm/model_executor/layers/fused_moe/runner/moe_runner.py: platform coupling 37 -> 39. That comes from #1254 (merged 2026-10-24), it is the same on main, and this PR leaves it unchanged. Fixing it needs the round-cost recorder hook moved behind the platform boundary, which belongs in its own change.Not run: DeepSeek-V4.1 serving. Its behaviour is unchanged by construction under
VLLM_V41_PACKED_KV=1. Whether its DSpark context has the same exposure was not checked.🤖 Generated with Claude Code