Repository navigation
multi-node dp support - #1603
Conversation
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
There was a problem hiding this comment.
Pull request overview
This PR adds native multi-node data-parallel (DP) serving support to ATOM by separating global DP ranks (for distributed groups/collectives) from local DP ranks (for GPU/NUMA placement), and introducing a coordinator/worker model where only global DP rank 0 serves the HTTP API while other nodes run EngineCore workers and connect back to rank 0.
Changes:
- Extends DP topology/config to support multi-node deployments (global vs local DP size/rank, master IP/ports) and validates topology in
ParallelConfig. - Updates EngineCore/CoreManager launch and routing to support multi-node socket addressing and rank mapping, including DP-attention rank flattening behavior.
- Adds CLI/kwargs backward-compat plumbing so callers can configure DP topology consistently via
ParallelConfig(with fallback for older loose kwargs).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
atom/utils/envs.py |
Adds ATOM_DP_SIZE_LOCAL env var for local DP topology override. |
atom/model_engine/model_runner.py |
Passes local_rank into distributed init to ensure correct device selection under multi-node DP. |
atom/model_engine/llm_engine.py |
Maintains backward compatibility for older callers passing DP topology via loose kwargs instead of ParallelConfig. |
atom/model_engine/engine_core.py |
Updates DP init assertions to allow global/local rank decoupling. |
atom/model_engine/engine_core_mgr.py |
Implements multi-node DP socket planning, global/local rank assignment, and coordinator vs worker behavior. |
atom/model_engine/async_proc.py |
Fixes NUMA/GPU mapping to use local DP rank when available. |
atom/model_engine/arg_utils.py |
Adds CLI flags and builds a ParallelConfig encapsulating multi-node DP topology. |
atom/config.py |
Makes data_parallel_size_local optional with default-to-global behavior, adds multinode detection, hashing, env overrides, and topology validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6bb67ea to
99188ff
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
atom/model_engine/engine_core_mgr.py:41
- PR description “EngineCore transport” specifies 2 ports per engine (
base + 2*r), but the implementation and docs now allocate 3 ports per engine (request/output/control). Please update the PR description to match the current 3-port plan so future readers don’t misconfigure ports.
# Engine sockets for a multi-node run cannot use IPC paths, and the two ends
# must derive identical TCP ports without negotiating. Both compute this plan
# from the shared master port.
DP_SOCKET_PORT_OFFSET = 100
DP_PORTS_PER_ENGINE = 3
docs/distributed_guide.md:514
- The guide claims “explicit flags win”, but
ParallelConfig.__post_init__appliesATOM_DP_*environment overrides after initialization, so a set env var will override any CLI-provided value. Please update the docs (or adjust precedence in code) so users don’t get surprising DP topology values.
Each has an `ATOM_DP_*` environment equivalent (`ATOM_DP_SIZE_LOCAL`,
`ATOM_DP_RANK`, …) for launch scripts; explicit flags win.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
atom/utils/init.py:103
- This docstring says
CoreManagerwraps engine spawns inset_device_control_env_var, butset_device_control_env_var()is no longer called anywhere in the codebase. The reference toCoreManageris now misleading; update the wording to describe the current (rollout-only) consumer without implying the DP serving path uses it.
Inert for the DP path that calls it, and must stay that way. It sets a
variable named by the literal placeholder string; the only reader is
``RLHFModelRunner.DP_DEVICE_MAP_ENV`` (atom/rollout/model_runner_ext.py),
which consults it solely when ``data_parallel_size <= 1``. ``CoreManager``
only wraps a spawn in this when ``data_parallel_size > 1``, so the two
never meet. Do not delete the placeholder string as dead -- it is a live
atom/model_engine/engine_core_mgr.py:40
- PR description (“EngineCore transport” section) still describes a 2-ports-per-rank plan, but the implementation and docs now use 3 ports per engine (input/output/control) via
INTERNODE_DP_PORTS_PER_ENGINE = 3. Please update the PR description to avoid confusing reviewers/operators about required firewall openings and port coordination.
# Engine sockets for a multi-node run cannot use IPC paths, and the two ends
# must derive identical TCP ports without negotiating. Both compute this plan
# from the shared master port.
INTERNODE_DP_SOCKET_PORT_OFFSET = 100
INTERNODE_DP_PORTS_PER_ENGINE = 3
docs/distributed_guide.md:514
- This sentence claims “explicit flags win”, but
ParallelConfig.__post_init__currently appliesATOM_DP_*overrides whenever the env var is set, even if the value was provided via CLI flags/programmatic config. Either adjust the precedence behavior in code, or update this doc to match reality (env overrides when set).
Each has an `ATOM_DP_*` environment equivalent (`ATOM_DP_SIZE_LOCAL`,
`ATOM_DP_RANK`, …) for launch scripts; explicit flags win.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/distributed_guide.md:514
- This claims CLI flags override env vars, but
ParallelConfig.__post_init__overwrites DP fields fromATOM_DP_*whenever those env vars are set. As written, a set env var will override an explicit flag value.
Each has an `ATOM_DP_*` environment equivalent (`ATOM_DP_SIZE_LOCAL`,
`ATOM_DP_RANK`, …) for launch scripts; explicit flags win.
docs/distributed_guide.md:545
- The PR description’s port formula uses 2 ports per rank (
+ 2*r/+ 2*r + 1), but the implementation and this doc section use 3 ports per engine (requests/outputs/control,base + 3r + {0,1,2}). Please update the PR description/design section to match the 3-port scheme so operators don’t misconfigure firewalls.
Engine sockets are derived from the DP master port; both ends compute them
independently and must agree. With `base = master_port + 100`, DP rank `r` uses:
| Port | Purpose |
|---|---|
| `base + 3r` | requests (ROUTER/DEALER) |
| `base + 3r + 1` | outputs (PULL/PUSH) |
| `base + 3r + 2` | control — utility commands, abort, shutdown |
atom/utils/init.py:104
- This docstring still states that
CoreManagerwraps engine spawns in this context manager whendata_parallel_size > 1, butCoreManagerno longer imports or usesset_device_control_env_varafter the multi-node DP changes. That makes the current explanation about why RLHF and DP paths “never meet” inaccurate.
``RLHFModelRunner.DP_DEVICE_MAP_ENV`` (atom/rollout/model_runner_ext.py),
which consults it solely when ``data_parallel_size <= 1``. ``CoreManager``
only wraps a spawn in this when ``data_parallel_size > 1``, so the two
never meet. Do not delete the placeholder string as dead -- it is a live
protocol constant for the rollout adapter.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/distributed_guide.md:527
- The multi-node example commands omit
--data-parallel-base-port. In multi-node runs the defaultdata_parallel_base_portis chosen dynamically (viaget_open_port()), so each node can pick a different port and distributed initialization can hang. The example should set an explicit base port on both nodes (matching the PR description and the CLI help inatom/model_engine/arg_utils.py).
python -m atom.entrypoints.openai_server --model <model> -tp 2 \
--data-parallel-size 8 --data-parallel-size-local 4 --data-parallel-rank 0 \
--data-parallel-master-ip 10.0.0.1 --data-parallel-master-port 29500
docs/configuration_guide.md:259
- The table now documents
data_parallel_size_localasNone → data_parallel_size, but the Computed property section immediately below still listsworld_size/world_size_across_dp. Those fields/properties were removed fromParallelConfig(seeatom/config.py), so this section is now misleading and should be updated to reflect the current API (e.g.,is_multinode_dp).
| `data_parallel_size_local` | `int \| None` | `None` → `data_parallel_size` | DP ranks on **this** node. Defaults to the global size, i.e. single-node. Set it lower to give a node one slice of a multi-node run; also reaches MoRI as `gpu_per_node`. Overridden by `ATOM_DP_SIZE_LOCAL` |
| `data_parallel_rank` | `int` | `0` | First **global** DP rank owned by this node; overridden by `ATOM_DP_RANK` |
| `data_parallel_rank_local` | `Optional[int]` | `None` | Local rank within the data-parallel group (SPMD mode); overridden by `ATOM_DP_RANK_LOCAL` |
docs/distributed_guide.md:542
- The PR description’s port formula describes 2 ports per rank (
+ 2*r/+ 2*r + 1), but the implementation and this guide use 3 ports per engine (requests/outputs/control viaINTERNODE_DP_PORTS_PER_ENGINE = 3). Please reconcile the PR description with the actual 3-port plan so operators don’t open the wrong firewall rules.
Engine sockets are derived from the DP master port; both ends compute them
independently and must agree. With `base = master_port + 100`, DP rank `r` uses:
| Port | Purpose |
|---|---|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (4)
docs/distributed_guide.md:527
- Same as node 0: the worker-node command should set
--data-parallel-base-portto the same value as the coordinator so model-runner distributed initialization uses a consistent rendezvous endpoint across nodes.
python -m atom.entrypoints.openai_server --model <model> -tp 2 \
--data-parallel-size 8 --data-parallel-size-local 4 --data-parallel-rank 4 \
--data-parallel-master-ip 10.0.0.1 --data-parallel-master-port 29500
atom/config.py:805
ParallelConfig.data_parallel_rankis now used as the first global DP rank owned by this node in multi-node mode (seeis_multinode_dplogic and CLI/docs). The field docstring still describes it as the in-group rank, which is misleading for readers and future callers.
data_parallel_rank: int = 0
"""Rank of the data parallel group."""
docs/distributed_guide.md:522
- The multi-node example omits
--data-parallel-base-port. In multi-node runs the model-runner distributed init usesParallelConfig.data_parallel_base_port, and if it’s left unset each node will pick a different ephemeral port, causing NCCL rendezvous failures/hangs. Include an explicit base port (must match on all nodes).
This issue also appears on line 525 of the same file.
python -m atom.entrypoints.openai_server --model <model> -tp 2 \
--data-parallel-size 8 --data-parallel-size-local 4 --data-parallel-rank 0 \
--data-parallel-master-ip 10.0.0.1 --data-parallel-master-port 29500
atom/model_engine/engine_core_mgr.py:40
- The PR description’s "EngineCore transport" section describes only 2 ports per engine rank (
base + 2*r/base + 2*r + 1). The implementation (and docs) now use 3 ports per engine (requests/outputs/control) withINTERNODE_DP_PORTS_PER_ENGINE = 3. Please update the PR description to match the actual socket plan so the design doc doesn’t diverge from code.
# Engine sockets for a multi-node run cannot use IPC paths, and the two ends
# must derive identical TCP ports without negotiating. Both compute this plan
# from the shared master port.
INTERNODE_DP_SOCKET_PORT_OFFSET = 100
INTERNODE_DP_PORTS_PER_ENGINE = 3
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (6)
docs/distributed_guide.md:527
- Multi-node launches require a stable
data_parallel_base_portshared across nodes (it’s used to builddistributed_init_method). This example omits--data-parallel-base-port, which can lead to different nodes choosing different ports and hanging during distributed init.
python -m atom.entrypoints.openai_server --model <model> -tp 2 \
--data-parallel-size 8 --data-parallel-size-local 4 --data-parallel-rank 4 \
--data-parallel-master-ip 10.0.0.1 --data-parallel-master-port 29500
docs/distributed_guide.md:514
- Docs claim that DP-related environment variables are only equivalents and that explicit flags win, but
ParallelConfig.__post_init__applies env var overrides whenever they are set. This is likely to confuse launches (e.g., a staleATOM_DP_RANK/ATOM_DP_SIZE_LOCALin the environment silently overriding CLI args). Please update the precedence wording to match the actual behavior.
Each has an `ATOM_DP_*` environment equivalent (`ATOM_DP_SIZE_LOCAL`,
`ATOM_DP_RANK`, …) for launch scripts; explicit flags win.
docs/distributed_guide.md:522
- Multi-node launches require a stable
data_parallel_base_portshared across nodes (it’s used to builddistributed_init_method). This example omits--data-parallel-base-port, which can lead to different nodes choosing different ports and hanging during distributed init.
This issue also appears on line 525 of the same file.
python -m atom.entrypoints.openai_server --model <model> -tp 2 \
--data-parallel-size 8 --data-parallel-size-local 4 --data-parallel-rank 0 \
--data-parallel-master-ip 10.0.0.1 --data-parallel-master-port 29500
docs/distributed_guide.md:545
- The PR description's EngineCore port formula describes 2 ports per engine (
+ 2*r), but the implementation/docs here use 3 ports per engine (requests/outputs/control). Please align the PR description (or the implementation) so operators don’t provision the wrong firewall rules/port ranges.
Engine sockets are derived from the DP master port; both ends compute them
independently and must agree. With `base = master_port + 100`, DP rank `r` uses:
| Port | Purpose |
|---|---|
| `base + 3r` | requests (ROUTER/DEALER) |
| `base + 3r + 1` | outputs (PULL/PUSH) |
| `base + 3r + 2` | control — utility commands, abort, shutdown |
atom/model_engine/arg_utils.py:663
- In inferred multi-node DP mode (
data_parallel_size_local < data_parallel_sizeordata_parallel_rank > 0), leavingdata_parallel_base_portunset means each node will fall back to a different ephemeral port, and distributed init can hang. Consider validating this at CLI/config construction time and raising a clear error unless--data-parallel-base-port(orATOM_DP_BASE_PORT) is provided.
# DP topology -> ParallelConfig. `data_parallel_size` stays in kwargs
# too: LLMEngine still reads the loose kwarg on the legacy path.
parallel_config_kwargs = {
"data_parallel_size": kwargs["data_parallel_size"],
"data_parallel_size_local": kwargs.pop("data_parallel_size_local"),
"data_parallel_rank": kwargs.pop("data_parallel_rank"),
"data_parallel_master_ip": kwargs.pop("data_parallel_master_ip"),
"data_parallel_master_port": kwargs.pop("data_parallel_master_port"),
}
base_port = kwargs.pop("data_parallel_base_port")
if base_port is not None:
parallel_config_kwargs["data_parallel_base_port"] = base_port
kwargs["parallel_config"] = ParallelConfig(**parallel_config_kwargs)
atom/model_engine/engine_core_mgr.py:68
- New multi-node DP helpers/logic here (deterministic TCP port planning, global↔local rank assignment, coordinator vs worker behavior) are not covered by existing unit tests (no references in
tests/tobuild_internode_dp_socket_plan,iter_dp_rank_assignments, oris_multinode_dp). Given the operational risk of hangs/misrouting, please add CPU-only tests for these helpers and for routing state sizing withglobal_engine_count.
def build_internode_dp_socket_plan(
*, engine_count: int, master_port: int
) -> list[InternodeDPSocketPlan]:
"""Derive deterministic per-engine TCP ports from the DP master port.
Multi-node only -- ``CoreManager.__init__`` calls this when
``is_multinode_dp`` and passes ``None`` otherwise, and that ``None`` is what
selects the IPC path (and, with it, the connect/bind polarity) further down.
Three ports per engine: requests, outputs, and control. Control is separate
so the request socket keeps a single writer thread (see _send_request).
"""
Signed-off-by: ganyi <ygan@amd.com>
Signed-off-by: ganyi <ygan@amd.com>
Signed-off-by: ganyi <ygan@amd.com>
2abc291 to
8d17022
Compare
PR ROCm#1603 parameterises multi-node DP as data_parallel_size_local plus a data_parallel_rank offset, not nnodes/node_rank. Add from_parallel_config so the two do not become competing CLI surfaces for one concept, and stop storing raw -tp/-dp: CoreManager's fold makes them unrecoverable, and a second copy of an authoritative field is how the two drift apart. Fix ep_size and gpu_per_node outside DP-attention. FusedMoEParallelConfig only flattens every rank into one EP group under DP-attention; otherwise ep_size stays at tp_size and there are dp_size independent groups. The view reported global_dp_size unconditionally, so -tp 8 -dp 2 read as ep=2/gpu_per_node=2 instead of 8/16. The old test missed it by asserting the multinode identity, which only checks the view against itself. Reject ragged node splits: EPLB's hierarchical placement asserts num_gpus % num_nodes == 0, so a split that cannot be expressed as equal slices has to fail at construction rather than inside a rebalance.
EPLBManager._nnodes was hardcoded to 1, so hierarchical placement never ran and expert groups were free to straddle nodes. PR ROCm#1603 made the derivation possible: data_parallel_size_local now defaults to data_parallel_size instead of 1, so the ratio reads 1 on a single node rather than dp_size. Wired outside the best-effort eplb_config block on purpose. A wrong node count does not fail — it spreads groups across nodes and makes every rebalance pay inter-node traffic for the rest of the run, which is only visible as unexplained latency. Cross-check ep_size against the topology for the same reason. --fake-eplb is the one case the DP fields cannot answer: it shrinks data_parallel_size_local to the visible device count while data_parallel_size keeps the simulated width, which is byte-identical to node 0 of a real two-node slice. from_parallel_config now refuses that input instead of reporting a node count that does not exist, and the caller handles the simulated case explicitly (nnodes=1, today's behaviour, so no change to fake-eplb benchmarks).
Multi-node narrows the configuration surface rather than adding an orthogonal dimension: most parallelism knobs reshape the rank space, and none of these combinations have run across a node boundary. Deny by default, widen as each is verified. What this avoids is not a crash but a run that starts, serves, and is quietly wrong -- or hangs with nothing in the log. Rejected: no DP-attention, no expert parallelism, PP > 1, PCP/DCP > 1, --moe-backend mega (FlyDSL raises NotImplementedError for internode), --enable-rapidserve (it selects DisaggCoreManager, a second rank layout), and the plugin path (its own multi-node world, plus plugin/config.py assigns data_parallel_size_local the global size). TBO warns instead: it should work once kernel selection is right, but is not verified. Each message carries why and what to do. A validator that only reports "invalid" moves the work to whoever reads the traceback, and for the hang-shaped failures here that is the expensive path. Two rendezvous rules ROCm#1603 left open: - An unset --data-parallel-base-port is now rejected. Its default is get_open_port(), evaluated per process, so nodes silently pick different ports and wait on addresses nobody bound. - Loopback master IP warns rather than rejects. Gate 1 runs both logical nodes on one box, where loopback is correct; rejecting it would block our own single-machine verification path. Also adds the divisibility check ROCm#1603 is missing: data_parallel_size % data_parallel_size_local. EPLB's hierarchical placement asserts num_gpus % num_nodes == 0, so a ragged split passes startup and then fails inside a rebalance thousands of steps later. Tests UNRUN -- no machine reachable this session.
gpu_per_node reaches MoRI as the node width, and InterNodeV1 computes its RDMA/XGMI peer targets from it. A wrong value does not raise -- it addresses the wrong ranks. Before ROCm#1603 it was 1 on every run and nothing noticed, because the intra-node kernel never reads it. So check it where it is built, against the topology: ep_size has to partition into exactly nnodes groups of gpu_per_node. Only under DP-attention, which is the case where every rank joins one EP group; otherwise EP spans a TP group and the identity does not hold. MoRI reads its heap settings when the heap is created, inside the first collective, so mori_env applies them before init_dist_env. It sets MORI_SHMEM_HEAP_SIZE (InterNodeV1 stages through five buffers where intra-node uses one) and MORI_EP_LAUNCH_CONFIG_MODE, and never overrides what an operator exported -- they chose against a specific fabric and this knows less than they do. MORI_SOCKET_IFNAME=lo is rejected across nodes. A single-node run needs that value, which is exactly how it survives into a multi-node launcher, and wrong it hangs the first collective with nothing in the log. node_count() is now the single place that answers "how many nodes", including the one case the DP fields cannot: --fake-eplb runs one box pretending to be wider, which reads identically to node 0 of a real split. EPLB now goes through it too. 41 CPU tests pass (topology + mori_env, neither needs torch). The torch-dependent EPLB tests remain unrun -- no machine reachable.
Under dp-attention SGLang starts one scheduler process per GPU, and the plugin built each process's ATOM ParallelConfig with its own global DP rank as data_parallel_rank and the global DP size as data_parallel_size_local. Since multi-node DP support (#1603), ParallelConfig's constructor reads data_parallel_rank as the start of the node's slice and rejects rank + size_local > size, so every rank but 0 failed at server start: ValueError: data_parallel_rank (3) + data_parallel_size_local (8) must not exceed data_parallel_size (8): this node's slice would run off the end of the global DP group Build the config for the single-node slice starting at rank 0 and assign the process rank afterwards, as EngineCoreManager does for native ATOM. The regression test lives under tests/ rather than tests/plugin/, which the CPU unit-test job skips, and uses the real ParallelConfig.
Summary
Add native multi-node data-parallel serving to ATOM.
Global DP ranks can now be distributed across multiple nodes. Global DP rank 0
acts as the sole serving coordinator, while other nodes launch their local
EngineCore workers and connect back to rank 0.
Multi-node mode is inferred directly from the configured DP topology. No
additional
distributed_dpordistributed_dp_servingflags are required.Motivation
The existing native DP implementation assumes every DP rank is local:
CoreManagerlaunches every DP rank;These assumptions do not hold when DP ranks span multiple nodes. For example,
a node that owns global DP ranks 8–15 still needs to map those workers to local
GPUs 0–7.
Design
DP topology
The topology is configured with:
--data-parallel-size,-dp--data-parallel-size-local--data-parallel-rank--data-parallel-master-ip--data-parallel-master-port--data-parallel-base-port--data-parallel-rank-localis assigned internally to EngineCore workers andnormally should not be set by users.
If
--data-parallel-size-localis omitted, it defaults to the global DP size,preserving existing single-node behavior.
Multi-node mode is inferred when:
The topology is validated as follows:
Coordinator/worker model
In multi-node mode:
routing hint.
Single-node deployments continue using the existing local IPC transport.
Global and local ranks
Each EngineCore receives two DP ranks:
For example, the second node in a DP16 deployment may be configured with:
Its rank mapping becomes:
This prevents global ranks on later nodes from being interpreted as local GPU
indices.
DP-attention rank mapping
With DP attention enabled, DP and TP ranks are flattened into EngineCore ranks:
The existing PCP + DP-attention incompatibility is unchanged.
EngineCore transport
The rank-0 coordinator binds one request and one output port for every effective
global EngineCore rank.
For rank
r:The socket plan stores only these semantic port values. The coordinator derives
wildcard bind addresses, while workers derive connect addresses using
data_parallel_master_ip.Both single-node IPC and multi-node TCP coordinator sockets are initialized
through the same binding path before workers start.
Backward compatibility
Existing single-node commands continue to work:
python -m atom.entrypoints.openai_server \ --model "$MODEL" \ -dp 8Because the local DP size is omitted, it defaults to 8 and all ranks are
launched locally.
Verification
Completed static verification:
git diff --checkA full multi-node GPU serving and accuracy run is still required before merge.
The model path must be available on both nodes.
Node 0: coordinator, global DP rank 0
Node 1: worker, global DP rank 1
Send requests only to node 0:
Two nodes with TP4 on each node
This configuration creates two global DP replicas, each using four GPUs:
Node 0
Node 1
Four nodes with two DP ranks per node
For global DP8, TP1, with two GPUs per node: