fix(hcu): disable CustomAllreduce by default on PCIe (no XGMI), including TP=2 - #55
kabishou11 wants to merge 3 commits into
Conversation
PCIe-only HCU topologies previously warned and still enabled CustomAllreduce, including TP=2. That path can hard-lock the host. Fail closed to HCCL unless VLLM_HCU_ENABLE_PCIE_CUSTOM_ALLREDUCE=1. Fixes HYGON-AI#54
6947179 to
ffd3822
Compare
alexanderbin123
left a comment
There was a problem hiding this comment.
Code review summary: one high-priority safety gap was found; please address the inline finding before merge.
Verification performed against ffd3822: the targeted PCIe-gate/native-lifecycle/shutdown suite passes locally (20 passed), and git diff --check is clean. The repository's selected HCU hardware jobs are currently skipped pending trusted ready-hcu authorization, so the aggregate ci-gate remains failing.
| # this checks hardware and driver support for NVLink | ||
| assert current_platform.is_cuda_alike() | ||
| fully_connected = current_platform.is_fully_connected(physical_device_ids) | ||
| if not allow_custom_allreduce_for_topology(fully_connected): |
There was a problem hiding this comment.
[High] Gate the AITER custom-allreduce path too
This check is bypassed when VLLM_ROCM_USE_AITER=1. In CudaCommunicator.__init__, use_aiter_allreduce then becomes true because VLLM_ROCM_USE_AITER_CUSTOM_AR defaults to true; it constructs AiterCustomAllreduce, and this HCU CustomAllreduce is instantiated only when aiter_ar_comm is None. The AITER wrapper delegates directly to AITER and never evaluates allow_custom_allreduce_for_topology, so a PCIe TP=2 deployment can still select a custom-allreduce backend without VLLM_HCU_ENABLE_PCIE_CUSTOM_ALLREDUCE=1.
--disable-custom-all-reduce disables both paths, whereas this patch gates only one. That leaves the advertised PCIe fail-closed policy incomplete for a supported configuration; given that the reported failure mode is a host hard lock, this path needs an explicit decision before merge. Please enforce the topology/opt-in decision before choosing either custom-AR backend (or explicitly disable AITER custom AR on non-XGMI unless the new opt-in is set), and add coverage for VLLM_ROCM_USE_AITER=1.
VLLM_ROCM_USE_AITER=1 still constructed AiterCustomAllreduce on PCIe because CudaCommunicator selected that backend before the native CustomAllreduce topology gate. Apply the same fail-closed opt-in before either custom-AR backend is constructed.
|
@alexanderbin123 Thanks for the catch. AITER custom-allreduce is now behind the same PCIe/no-XGMI fail-closed opt-in as the native path ( |
Summary
Fixes #54
On PCIe-only HCU topologies (no XGMI),
CustomAllreducepreviously warned (We are using PCIe's custom allreduce) and then stayed enabled.should_custom_arstill selected the custom kernel forworld_size == 2whenfully_connectedwas false.On BW100 (
gfx936) PCIe, TP=2 + CustomAllreduce can hard-lock the host on the first chat request after health=200.--disable-custom-all-reduce(HCCL) is the stable path.This change:
is_fully_connectedis false, including TP=2.VLLM_HCU_ENABLE_PCIE_CUSTOM_ALLREDUCE=1.Validation
20 passed (no real DCU required for the topology gate):
is_fully_connected=False,world_size=2→ CustomAllreduce disabledVLLM_HCU_ENABLE_PCIE_CUSTOM_ALLREDUCE=1re-enables the PCIe path