feat(inference): Dynamo integration with prime-rl - #3176
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 322fd68. Configure here.
| def __init__(self, client_config: ClientConfig, workers: tuple[DiscoveredDynamoWorker, ...], **kwargs): | ||
| admin_clients = _setup_control_clients([worker.admin_base_url for worker in workers]) | ||
| super().__init__(client_config, admin_clients=admin_clients, **kwargs) | ||
| self._admin_world_sizes = [worker.world_size for worker in workers] |
There was a problem hiding this comment.
Unused per-engine world sizes
High Severity
Discovered per-engine world_size values are stored in _admin_world_sizes but never exposed or applied. init_nccl_broadcast and init_nixl_broadcast still assume equal GPUs per admin client, so heterogeneous Dynamo topologies (common in P/D) get incorrect rank_offsets and broken weight broadcast groups.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 322fd68. Configure here.
| timeout=httpx.Timeout(None), | ||
| ) | ||
| for url in urls | ||
| ] |
There was a problem hiding this comment.
Admin clients drop auth headers
Medium Severity
_setup_control_clients builds Dynamo admin AsyncClients without the API key, static headers, or headers_from_env that setup_admin_clients always attaches. Secured or header-gated admin endpoints will reject pause, resume, and weight-update calls even though discovery succeeded.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 322fd68. Configure here.
Co-authored-by: Matej Sirovatka <54212263+S1ro1@users.noreply.github.com> Signed-off-by: Biswa Panda <biswa.panda@gmail.com>


Summary
Why: Dynamo owns the inference workers and their admin endpoints, while Prime-RL normally knows only the frontend URL. Hard-coding worker URLs is fragile and does not provide an authoritative inference topology for weight synchronization.
Changes:
dynamo_discovery_urlalongside the existing inferencebase_url/v1/rl/workersweight_broadcast.inference_world_sizefor externally managed inferenceScope
This is the minimal root integration for Dynamo-managed inference. It follows current upstream behavior and does not restore the removed
pool_sizeplumbing.Test plan
ruff checkon config, orchestrator, client, discovery, and test filesNote
Medium Risk
Touches weight broadcast topology, NCCL initialization, and orchestrator startup paths; misconfigured inference_world_size or discovery could deadlock or mis-route admin/weight updates on external pools.
Overview
Adds Dynamo-managed external inference so Prime-RL can discover vLLM admin endpoints and topology without hard-coded
admin_base_urllists.Orchestrator clients gain optional
dynamo_discovery_url, mutually exclusive with elastic DNS discovery and staticadmin_base_url. When set,DynamoInferencePoolpolls/v1/rl/workers, validates worker identity and summedworld_size, and wires train/eval traffic throughbase_urlwhile weight/health admin calls hit discovered per-engine URLs.inference_world_sizeis now an explicit knob on shared/orchestrator weight-broadcast configs (filesystem, NCCL, NIXL). Withinference: null, RL resolution propagates that value into trainer/orchestrator broadcast settings instead of inferring from localparallel.dp * tp. Dynamo orchestrator configs must setweight_broadcast.inference_world_sizeexplicitly; NCCL’s “≥2 local GPUs” check is skipped when external world size is provided.Reviewed by Cursor Bugbot for commit 322fd68. Bugbot is set up for automated code reviews on this repo. Configure here.