Conversation
ishandhanani
left a comment
There was a problem hiding this comment.
The net diff is small (9 files, ~180 lines) and the scope discipline is good: GPU scheduling stays on use_gpus_per_node_directive / default_sbatch_directives, and the sidecar special case in worker_stage.py was only moved, not added. The concern is that the design center, accelerator_vendor, is a closed enum whose two effects (pick a visibility env var, suppress the DCGM default) are both hardcoded in Python, when each already has a cluster-config pattern in this repo. srtslurm is meant to be portable across clusters by editing srtslurm.yaml, not by adding vendor branches. Inline comments have the specifics.
Suggested shape instead of the enum:
visible_devices_env: str = "CUDA_VISIBLE_DEVICES"onClusterConfig, read once intoRuntimeContext, same asnetwork_interface. An AMD profile setsROCR_VISIBLE_DEVICES. Noaccelerator.py, noValueErrorpath, noLiteralto keep in sync across three files.default_gpu_exporter: TelemetryExporterConfig | NoneonClusterConfig, followingdefault_health_check/default_host_setup. An AMD profile points it at AMD's device-metrics-exporter once; NVIDIA clusters keep the built-in DCGM default;observability.tachometer.dcgm_exporterin a recipe still wins. The schema property then stops reading global config.
Not in the diff, but worth noting for a PR titled base AMD support:
src/srtctl/core/fingerprint.py:330and:917shell out tonvidia-smi;:353-363callnvccandtorch.cuda.nccl.version(). On AMD the fingerprint and lockfile silently record GPU, CUDA, and NCCL as unavailable.srtslurm.yaml.exampleis not updated here (#432 adds the line).- The
CLAUDE.mdhunk is a trailing-newline removal only.
Housekeeping:
- The first commit touched ~30 files and the second reverted nearly all of them. Please squash so the history matches what lands.
- CI
conventional-titleis red on the[AMD Support][1/3]prefix.
bb1efc3 to
dd26457
Compare
0c6bf97 to
a9ebc67
Compare
The GPU-mask test never branched on the variable name, so parametrize over the behavioral cases only. Drop the duplicate dry-run mask value and an assertion already implied by exact equality, and check both resolved control-plane IPs on the runtime. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
a9ebc67 to
d804d1c
Compare
Brings in NVIDIA#486 (RuntimeContext.container_log_dir), so the branch's own _container_log_path helper and its Path import are dropped in favor of the shared accessor; the config dump test is now identical on both sides. Signed-off-by: Ishan Dhanani <idhanani@nvidia.com>
main now routes every backend question through BackendProtocol and every frontend decision through the frontend class, so the AMD runtime changes land on those seams instead of beside them: the visible-devices hook is the protocol member should_set_visible_devices() on every backend (vLLM answers from set_visible_devices, the others True), the worker stage builds the mask from runtime.visible_devices_env in one place, and the static router's pre-start worker probe reads config.health_check and runtime.srun_options directly instead of probing with getattr. Signed-off-by: Ishan Dhanani <idhanani@nvidia.com>
|
Merged What moved, and why:
Validation on the merged head: full suite 3034 passed on Python 3.13 and 3.10, |
…M Router Merge of upstream/amd-runtime (main + NVIDIA#431) into this branch, with the MoRI-IO integration rebuilt on the seams main now has instead of beside them. What the feature does is unchanged: engine.connector: moriio runs a ROCm prefill/decode pair over AMD's MoRI-IO connector with the vLLM Router in ZMQ discovery mode. Backend (backends/vllm.py) - The connector table rows are KVConnector(kv_connector, kv_role, module_path, discovery). "moriio" is a row with discovery=True and a mode-dependent kv_role; no connector name is compared in a branch. - connector_for_mode(mode) is the one resolver for engine.connector and the roles.<role>.args.connector override; kv_connector_for_mode and discovers_workers are built on it. All three command paths use it. - kv_transfer_config(mode, process, runtime) builds --kv-transfer-config. For a discovery connector the extra config carries proxy_ip (head node), proxy_ping_port, http_port, host_ip (the worker's own routable IP), handshake_port and notify_port (the keys moriio_common.py reads, vllm-project/vllm 9679173788), read_mode. Upstream's defaults of 6301 and 61005 for every worker collide on colocated P/D; a worker built without its listeners is refused rather than given them. - The allocator hands a discovery worker MORIIO_HANDSHAKE_PORTS and a MORIIO_NOTIFY_PORTS block of one port per local rank (upstream adds the DP and TP rank to the notify base) and no VLLM_PORT scan range, since MoRI-IO opens its other listeners inside TP children that inherit it. Process gains moriio_handshake_port and moriio_notify_port. Frontend (frontends/vllm_router.py, static_router.py) - StaticRouterFrontend.discovers_workers(backend) decides whether the P/D URLs go on the command line; build_router_command takes the backend. - VLLMRouterFrontend adds --kv-connector <name> --vllm-discovery-address 0.0.0.0:36367 (RouterArgs in vllm-project/router), advertises no NIXL bootstrap port, and validates the discovery rules: both roles on the connector, a P/D topology, one router on the head node. - Readiness is the Router's own /health, 503 until a prefill and a decode registered (vllm-project/router 43140bc8e2, VllmPDRouter::health), through probe_ready. probe_ready now receives the recipe on every frontend, and wait_for_model passes it; the completions POST in core/health.py is gone. - The schema refuses a discovery connector with any other frontend. Docs, example, tests - docs/vllm-router.md gains a MoRI-IO discovery section with the contract table; config-reference.md points at it (the duplicate section is gone); ports tables in CLAUDE.md and docs/architecture.md list the two kinds. - examples/vllm/vllm-router-moriio-disagg.yaml, colocated P/D, is in the examples matrix and in every example-driven test. - Tests cover the worker JSON against the realized topology, the refusal without listeners, colocated listener allocation, the role override, the discovery command, the bootstrap port, the /health probe, and the recipe rules. Signed-off-by: Ishan Dhanani <idhanani@nvidia.com>
Signed-off-by: Ishan Dhanani <idhanani@nvidia.com> # Conflicts: # docs/schema-reference.md
Scope
Enable AMD hardware and make native srt-slurm jobs work on our Slurm clusters. This combines the former hardware and required-runtime PRs; functionality needed to launch the deployment is no longer hidden in a separate prerequisite layer.
set_visible_devicesoption.Upstream scheduling escape hatches handle GRES:
use_gpus_per_node_directive: falseanddefault_sbatch_directives: {gres: "gpu:8"}. Cluster host setup uses the existing lifecycle. No engine patch, new deployment system, automatic network fallback reordering, or Dynamo installation workaround is added.Review map
core/schema.py,core/config.py,core/runtime.py: cluster defaults and runtime context.cli/mixins/worker_stage.py,backends/vllm.py: GPU binding and container-visible config.frontends/,services/implicit.py: configured addresses, Slurm options, and native startup.Manual stack
#431 AMD hardware + required cluster runtime → #432 vLLM/MoRI → #433 ATOM/AToMesh → #476 optional observation and diagnostics.
The core runtime does not depend on #476. The InferenceX CI launcher uses its optional
srtctl waitcommand to stream logs and report allocation completion; it is not part of GPU/worker/router enablement. ATOM uses Mooncake and has no engine dependency on MoRI.Optional deployment fork #16, Dynamo installation fork #15, and Infera fork #3 are outside this validation.
E2E validation
InferenceX #2555 targets the seven-point MI355X DeepSeek V4 AgentX sweep plus three default evaluations.
Current full validation: InferenceX run 35231968727, port commit
15e61d4e865c9eb8c64009f57ec0948e4c934e12, fork runtimec63427249ab996bd38d364cad1629aa5aa8877ef. This preserves the maintainer-selected DSPARK baseline #3188, including all seven points and real-acceptance evaluation. Not yet green.Previous attempt 35181888533 passed four throughput points and c16 GSM8K (97.35% versus 91% threshold). Three points stopped during host preparation, and two evals exposed a JSON-environment templating defect in the optional runtime layer. The replacement removes that formatting, uses native eval artifacts, and restores the existing GPU-drain time allowance. Serving settings are unchanged. #3170 is closed as superseded.
The reorganized revision is not E2E validated. These upstream PRs are synchronized for review; a complete green run and artifact inspection are still pending.
Latest completed runs before this cleanup/rebase, retained as historical evidence only:
All four fork PRs target upstream
mainand are manually stacked: #431 → #432 → #433 → #476. Each later branch includes the preceding layers; merge in order and rebase the remaining PRs.