feat(vllm): split single and distributed suites - #372
Conversation
227abf0 to
c596192
Compare
solaiys
left a comment
solaiys
left a comment
There was a problem hiding this comment.
Two merge-blocking correctness issues: the new cell_key(..., nnodes=...) contract is not applied on the Run Deck path (distributed threshold specs will not attach), and vllm_distributed uses every cluster host while every shipped recipe/threshold still assumes a 2-node job (PP=2, NNODES=2).
Inline comments have the concrete fixes.
| if int(self.params.pipeline_parallel_size) > 1: | ||
| base += f"PP={self.params.pipeline_parallel_size}," | ||
| pp = int(pipeline_parallel_size if pipeline_parallel_size is not None else self.params.pipeline_parallel_size) | ||
| if int(nnodes) > 1: |
There was a problem hiding this comment.
Run Deck still looks up the old 3-arg cell key, so distributed thresholds will not attach. PP= / NNODES= are added only when the caller passes nnodes>1. The suite fixture and _test_metric do that, but cvs/lib/report/cell_build.py still calls variant_config.cell_key(isl, osl, conc) (defaults nnodes=1). For a distributed cell that becomes ISL=…,TP=8,CONC=16 while the threshold file is ISL=…,PP=2,NNODES=2,CONC=16, so the deck treats every gated metric as missing/record-only.
This is a regression versus main, where cell_key(isl, osl, conc) used params.pipeline_parallel_size and still emitted PP= for distributed configs.
Fix: record the effective topology on the variant in the vllm_targets fixture (e.g. _effective_nnodes / _effective_pp) and have cell_key use those when the kwargs are omitted, or teach CellRecordBuilder.build_one to pass nnodes / pipeline_parallel_size without breaking SGLang’s 3-arg cell_key. Add a unit test that cell_key(isl, osl, conc) matches expected_cells(nnodes=N, …) after the fixture has run.
| raise ValueError("vllm_distributed requires pipeline_parallel_size>1 unless distributed-executor-backend=ray") | ||
| if not variant.roles.server.ib_netdev: | ||
| raise ValueError("vllm_distributed requires roles.server.ib_netdev on multi-host clusters") | ||
| return (hosts,), effective_pp |
There was a problem hiding this comment.
vllm_distributed launches on every cluster host, but every shipped recipe is a 2-node job. This returns the full orch.hosts tuple. VllmJob then sets --nnodes to that length while configs keep pipeline_parallel_size=2 and every *_distributed_threshold.json is keyed …,PP=2,NNODES=2,….
A normal CVS cluster.json has N>2 nodes (the templates say to add entries to scale). On an 8-node cluster this will (1) start an 8-node vLLM world with TP=8, PP=2 (GPU count 64 vs TP×PP=16 — serve fails or mis-shards), and (2) look up NNODES=8 keys that are not in the threshold file. validate_thresholds_cover_sweep only errors when enforce_thresholds is true; shipped configs have it false, so this is a warning plus a bad launch rather than a hard stop.
SGLang already scopes the orchestrator to the hosts listed in the inference config rather than the whole cluster. Do the same here: restore a declared host count (or host list) and take the first N cluster nodes, or fail before launch when len(hosts) does not match the recipe (do not rely on enforce_thresholds). The one-host fallback can stay.
There was a problem hiding this comment.
The suite split itself is the right customer-facing change: cvs run vllm_single / vllm_distributed, first-host scoping for single, and sharing the existing vLLM Run Deck profile are all good.
I agree with @solaiys on the two merge blockers (not re-filing them):
- Run Deck still calls
cell_key(isl, osl, conc)so distributedPP=/NNODES=keys never attach. vllm_distributedtakes every cluster host while every shipped recipe/threshold is a 2-node job.
Please also address the remaining inline comments. I would not merge until (1), (2), and the VllmJob vs vllm_targets split of truth are fixed — otherwise a later "first N hosts" patch can look correct in fixtures and still launch on the full cluster.
Docs drift: cvs/lib/inference/utils/docs/cell-key-format.md and the utils AGENTS.md still describe the old 3-arg cell_key and load-time _check_thresholds_cover_sweep. The workloads README still hard-codes NNODES=2 while the suite now claims "all cluster hosts".
| self.ib_hcas = ib_hcas or [] | ||
| self.goodput_slo = goodput_slo | ||
| self.log_subdir = log_subdir | ||
| self.hosts = tuple(orch.hosts) |
There was a problem hiding this comment.
VllmJob does not consume vllm_targets. It snapshots orch.hosts and derives --nnodes / --pipeline-parallel-size from that length.
vllm_targets is what the suite fixture validates against the threshold matrix, and what _test_metric uses for cell_key(..., nnodes=len(vllm_targets[0])). If we later slice to the first N cluster hosts only in build_vllm_targets (the SGLang-style fix) without also scoping the orchestrator, the job will still bring up a world on every host.
Please drive self.hosts from the selected target group (pass it into VllmJob, or have the job take hosts= explicitly) so launch topology and threshold keys cannot diverge.
| nodes = scoped.get("node_dict") | ||
| if isinstance(nodes, dict) and nodes: | ||
| host, node = next(iter(nodes.items())) | ||
| scoped["node_dict"] = {host: node} |
There was a problem hiding this comment.
"First cluster host" here is next(iter(node_dict)), not head_node_dict.mgmt_ip. The unit test even uses a cluster whose head is node1 and asserts the scoped run uses node0.
That will surprise operators who set a specific head / jump host. Either document that vllm_single always uses JSON insertion order of node_dict (and that head_node_dict is overwritten), or prefer the configured head when it is present in node_dict.
| "_comment": "Uncalibrated placeholders for deepseek-r1-0528_fp8 (distributed, TP=8, PP=2, CONC=16). Every value is 0 and means 'not yet measured' -- covers client.*, gpu.* and prom.*. enforce_thresholds is false in the config, so these record without gating. Replace with measured values from a calibration run before enabling enforcement.", | ||
| "_comment_accuracy": "The accuracy block is keyed by accuracy task id, then by lm-eval metric key, so it cannot be pre-enumerated the way client/gpu/prom can -- its contents follow whichever tasks config.json selects in accuracy.tasks, which is empty. Shape to fill in alongside a task: \"accuracy\": {\"<task-id>\": {\"<lm_eval_task>.<metric>\": {\"kind\": \"min\", \"value\": 0}}}, where <metric> is the results.json key with commas replaced by __ (e.g. gsm8k.exact_match__strict-match). Exempt from the sweep-cell coverage check via NON_SWEEP_THRESHOLD_KEYS.", | ||
| "ISL=1024,OSL=1024,TP=8,PP=2,CONC=16": { | ||
| "ISL=1024,OSL=1024,TP=8,PP=2,NNODES=2,CONC=16": { |
There was a problem hiding this comment.
why is NNODES needed here? TP and PP will give you the topology.
There was a problem hiding this comment.
@amd-droy from what I understand, TP and PP say how the model is split across GPUs (tensor vs pipeline). They do not always tell you how many machines were in the run. Node count comes from the cluster file and which command you run (vllm_single vs vllm_distributed). The same config can end up on 1 host or 2, with different performance. So the threshold key adds NNODES=2 to mean: “these pass/fail limits are for a run that actually used 2 hosts”; not “only use 2 nodes” (the cluster and suite already control that).
For the usual case today such as for distributed, PP=2, two hosts, PP=2 and NNODES=2 often go together. NNODES is still useful so a 1-host fallback or a future multinode layout doesn’t accidentally reuse the wrong threshold row when TP/PP/CONC look the same.
It’s not redundant noise. Once nnodes is removed from the vLLM config, NNODES= in the key is how you tie a threshold row to the actual host count of that run (2-node distributed vs 1-node fallback, same config).
There was a problem hiding this comment.
already discussed. NNODES can be removed, TP*PP will give no of gpus in use.
| policy = seq_combo.get("policy", "default") if isinstance(seq_combo, dict) else "default" | ||
| return (model, gpu, str(isl), str(osl), str(policy), int(concurrency)) | ||
| name = seq_combo.get("name", "default") if isinstance(seq_combo, dict) else "default" | ||
| return (model, gpu, isl, osl, name, concurrency) |
There was a problem hiding this comment.
The result-tuple path dropped normalization: this used to be (model, gpu, str(isl), str(osl), str(policy), int(concurrency)). Raw isl/osl/concurrency will miss inf_res_dict when one side is "16" and the other is 16 (parametrize vs HTML extras), so the card shows empty actuals.
Make suite intent explicit, scope single-node execution to the first cluster host, and derive distributed topology from orchestrator hosts. Signed-off-by: Atul Nair <atnair@amd.com>
Resolve both explicit suite names through the existing vLLM report profile without duplicating report configuration. Signed-off-by: Atul Nair <atnair@amd.com>
Explain first-host single-node behavior, distributed fallback, and cluster-derived topology across the vLLM guides and workload catalog. Signed-off-by: Atul Nair <atnair@amd.com>
Keep launch, threshold lookup, and Run Deck cells on one topology identity, and reject unsupported distributed host counts before setup. Signed-off-by: Atul Nair <atnair@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Document the current one-host fallback and two-host distributed recipe contract, reserving larger placements for explicit validated configurations. Signed-off-by: Atul Nair <atnair@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Keep report lookup keys compatible with the consolidated vLLM config layout while preserving distinct distributed Ray topology keys without NNODES. Signed-off-by: Atul Nair <atnair@amd.com>
29fa7fb to
b165dbe
Compare
4 participants
Summary
vllmcommand with explicitvllm_singleandvllm_distributedsuitesvllm_singleto the first cluster host while preserving multi-host MP/Ray execution and one-host fallback forvllm_distributedcvs run vllmis intentionally removedTracking
Test plan
make fmt-checkmake lintmake ut(1,536 tests, 1 skipped)vllm_singlesample suite (55 tests)vllm_distributedsample suite (55 tests)