Skip to content

support tp>1 - #241

Merged
fangyuchu merged 1 commit into
feature/ft-simplifyfrom
fyc-dev
Jul 6, 2026
Merged

fangyuchu merged 1 commit into
feature/ft-simplifyfrom
fyc-dev

Conversation

@fangyuchu

@fangyuchu fangyuchu commented Jul 6, 2026 •

Copy link
Copy Markdown
Owner

Purpose

Test Plan

test with dp2 tp 2, and inject fault on DP rank 1 (on both tp ranks)

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: fangyuchu <fangyuchu@qq.com>

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

Copy link
Copy Markdown

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 updates the fault tolerance engine core and GPU worker sentinel to support multiple data parallel (DP) ports instead of a single port, using JSON serialization to store and retrieve the list of ports. However, the reviewer identified critical issues with this implementation: generating a port for every rank in the world size and indexing by the worker's rank is incorrect for multi-TP/PP configurations. Because ranks within the same DP group must share the same port to successfully initialize, the port allocation and indexing should instead be based on the number of independent DP groups (calculated via world size divided by data parallel size).

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 130 to 137
if parallel_config.data_parallel_rank == 0:
worker_port = get_open_port()
worker_ports = [get_open_port() for _ in range(parallel_config.world_size)]
engine_port = get_open_port()
engine.dp_store.set(worker_key, str(worker_port).encode())
engine.dp_store.set(worker_key, json.dumps(worker_ports).encode())
engine.dp_store.set(engine_key, str(engine_port).encode())
else:
worker_port = int(engine.dp_store.get(worker_key).decode())
worker_ports = json.loads(engine.dp_store.get(worker_key).decode())
engine_port = int(engine.dp_store.get(engine_key).decode())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

Generating world_size ports is redundant and incorrect. In a multi-TP/PP setup, the data parallel group is formed by ranks that share the same TP/PP/PCP indices. Therefore, there are world_size // data_parallel_size independent DP groups, and we only need one port per DP group. Generating world_size ports and indexing them by self.worker.rank will cause different ranks in the same DP group to use different ports, preventing them from connecting and causing initialization to hang.

Suggested change
if parallel_config.data_parallel_rank == 0:
worker_port = get_open_port()
worker_ports = [get_open_port() for _ in range(parallel_config.world_size)]
engine_port = get_open_port()
engine.dp_store.set(worker_key, str(worker_port).encode())
engine.dp_store.set(worker_key, json.dumps(worker_ports).encode())
engine.dp_store.set(engine_key, str(engine_port).encode())
else:
worker_port = int(engine.dp_store.get(worker_key).decode())
worker_ports = json.loads(engine.dp_store.get(worker_key).decode())
engine_port = int(engine.dp_store.get(engine_key).decode())
if parallel_config.data_parallel_rank == 0:
num_workers_per_dp_rank = parallel_config.world_size // parallel_config.data_parallel_size
worker_ports = [get_open_port() for _ in range(num_workers_per_dp_rank)]
engine_port = get_open_port()
engine.dp_store.set(worker_key, json.dumps(worker_ports).encode())
engine.dp_store.set(engine_key, str(engine_port).encode())
else:
worker_ports = json.loads(engine.dp_store.get(worker_key).decode())
engine_port = int(engine.dp_store.get(engine_key).decode())

Comment on lines +58 to +59
world_size = self.worker.parallel_config.world_size
port = params["new_stateless_dp_group_ports"][self.worker.rank % world_size]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

Using self.worker.rank % world_size resolves to self.worker.rank because the rank is always less than the world size. This assigns a unique port to every single worker. However, workers belonging to the same DP group must share the same port to initialize their process group. Indexing by self.worker.rank % num_workers_per_dp_rank ensures that all ranks in the same DP group get the same port.

            world_size = self.worker.parallel_config.world_size
            num_workers_per_dp_rank = world_size // self.dp_size
            port = params["new_stateless_dp_group_ports"][self.worker.rank % num_workers_per_dp_rank]

@fangyuchu
fangyuchu merged commit f55277b into feature/ft-simplify Jul 6, 2026
4 of 5 checks passed
fangyuchu added a commit that referenced this pull request Jul 14, 2026
Signed-off-by: fangyuchu <fangyuchu@qq.com>
fangyuchu added a commit that referenced this pull request Jul 20, 2026
Signed-off-by: fangyuchu <fangyuchu@qq.com>
fangyuchu added a commit that referenced this pull request Jul 23, 2026
Signed-off-by: fangyuchu <fangyuchu@qq.com>
zWaNg3 added a commit to zWaNg3/vllm that referenced this pull request Jul 25, 2026
…ft-simplify

This squashes the following commits into one:
- Fault Tolerance Framework (fangyuchu#229)
- unify param name for nixl and deepep and destroy old cpu_group in retry (fangyuchu#230)
- Surface exception to status
- change status report mode from pull to push
- simplify fault tolerance config args
- use existing get_all2all_manager implementation
- support tp>1 (fangyuchu#241)
- Enhance the pass of fault tolerance results (fangyuchu#242)
- clean states for model runner v2
- add support for fault detection through mask for model runner v2
- [FT] validate single API server, fix clean_buffers ordering and worker state cleanup
- [FT] make apply endpoint async and gate recovery by engine status
- add e2e test for fault tolerance
- add e2e test for retry recovery
- refactor e2e tests with concurrent polling and add Buildkite CI config (fangyuchu#250)
- set cpu timeout to default value of nixl-ep in test (fangyuchu#251)

Co-Authored-By: Claude <noreply@anthropic.com>
fangyuchu added a commit that referenced this pull request Jul 25, 2026
Signed-off-by: fangyuchu <fangyuchu@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant