fix(inference): map external engine ranks - #3177
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6fe4249. Configure here.
| f"{logical_data_parallel_size}" | ||
| ) | ||
|
|
||
| rank = rank_offset + data_parallel_index * model_parallel_size + worker_rank % model_parallel_size |
There was a problem hiding this comment.
Global DP index double-counts offsets
Medium Severity
global_inference_rank adds rank_offset to data_parallel_index * model_parallel_size, while dense EngineCores keep a globally numbered DP index and clients also assign per-engine rank_offsets. For external-LB topologies where each admin client is one DP replica, those contributions overlap, so ranks are rejected by the engine-local bounds check or land outside the intended span. Weight broadcast init then fails or collides once explicit engine_world_sizes are used.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 6fe4249. Configure here.
| global_rank_inference = rank_offset + self.device.index | ||
| else: | ||
| parallel_config = self.parallel_config | ||
| global_rank_inference = global_inference_rank( |
There was a problem hiding this comment.
Would be for having a single path be used here, we can maybe consolidate this?
| global_rank = rank_offset + self.device.index | ||
| else: | ||
| parallel_config = self.parallel_config | ||
| global_rank = global_inference_rank( |
| model_parallel_size = tensor_parallel_size * pipeline_parallel_size * prefill_context_parallel_size | ||
| logical_data_parallel_size = data_parallel_size | ||
| if engine_world_size is not None: | ||
| if engine_world_size <= 0 or engine_world_size % model_parallel_size: |
There was a problem hiding this comment.
These 3 ValueErrors seem to never be able to happen
| """ | ||
| logger = get_logger() | ||
|
|
||
| has_explicit_engine_world_sizes = engine_world_sizes is not None |
There was a problem hiding this comment.
again feels quite clunky by a way we have 2 separate paths, can we consolidate to 1 that is agnostic?
|
|
||
| gpus_per_server = inference_world_size // len(admin_clients) | ||
| if engine_world_sizes is None: | ||
| if inference_world_size % len(admin_clients) != 0: |
There was a problem hiding this comment.
These runtime checks seem quite eh, would again make it "always work" and be a single path for both raw vllm and dynamo
|
|
||
| async def initialize(admin_client: AsyncClient, rank_offset: int) -> None: | ||
| has_explicit_engine_world_sizes = engine_world_sizes is not None | ||
| if engine_world_sizes is None: |
There was a problem hiding this comment.
Same concern as above, is very weird to do these checks at runtime and also not a fan of the split path
2 participants
Summary
Why: An external inference endpoint can represent several TP/PP/DP ranks, so assigning one collective rank per admin URL is incorrect. Multi-engine weight transfer needs deterministic, non-overlapping rank spans or collectives can deadlock or update the wrong workers.
Changes:
Scope
This is backend-neutral topology support. It does not introduce Dynamo configuration or discovery.
Test plan
ruff checkon rank, client, worker, and test filespytest -q tests/unit/inference/test_nccl_rank.py tests/unit/utils/test_external_engine_topology.pyNote
Cursor Bugbot is generating a summary for commit 6fe4249. Configure here.