test(vllm): fix 3 stale validate_args tests for the post-#85 per-engine-TP contract - #91
Merged
Conversation
…e-TP contract PR #85 removed the global vllm_tp_size from validate_args (TP is derived per-engine in vllm_engine._resolve_vllm_parallel_sizes) and moved the pp-divisibility check out of validate_args. But three test_arguments.py cases still asserted the old contract and now fail on feature/multi_nodes: - test_validate_args_pp1 (expected ns.vllm_tp_size == 4) - test_validate_args_pp2_dp2_derives_tp (expected ns.vllm_tp_size == 2) - test_validate_args_pp_indivisible_asserts (expected validate_args to raise) Rewrite them to the new contract: validate_args records pp/dp but sets no global TP, and no longer raises on pp-indivisibility (that check moved per-engine, covered in test_vllm_engine.py). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the unit tests in test_arguments.py to reflect changes in the validate_args function. Specifically, validate_args no longer sets a global vllm_tp_size or raises an assertion error for indivisible pipeline parallel sizes, as these responsibilities have been moved to the per-engine resolver. There are no review comments, and I have no feedback to provide.
aoshen02
added a commit
that referenced
this pull request
May 31, 2026
* [Feature][1/N] Add vLLM multi-node rollout engine topology - VllmEngineTopology / compute_server_args for cross-node vLLM engines - Merge origin/main: add processed_logprobs default and weight-transfer comments - Fix _response_json for vLLM sleep/wake empty HTTP 200 body - Update unit tests for multi-node SKIPPED_DESTS and vllm_router_ip API * fix(vllm): per-engine TP + strict external config check (harden #68) (#85) * fix(vllm): derive rollout-engine TP per-engine + strict external config check Two fixes of one root cause — TP/parallel sizing read the *global* rollout_num_gpus_per_engine instead of the per-engine value — across the two engine launch paths. P2 (managed launch): validate_args unconditionally set a global args.vllm_tp_size (= global rollout_num_gpus_per_engine // pp) and _resolve_vllm_parallel_sizes preferred it, so the per-engine `tp = gpus_per_engine // pp` branch was dead in real runs. A heterogeneous per-group engine (e.g. num_gpus_per_engine=2, tp=2) thus launched with the global TP while the trainer sized the NCCL weight-transfer rendezvous from the per-group engine_gpu_counts — they disagreed and the rendezvous hung 300s ("3/4 clients joined"). TP is now derived per engine in _resolve_vllm_parallel_sizes (no global shadow), matching upstream slime's sglang_engine; the global vllm_tp_size computation is removed. dp>1 raises NotImplementedError (DP/EP wiring is a follow-up); pp divisibility validated per engine. P1 (external engine): _wait_external_config_ready compared the engine's reported TP against the same global flag and only warned, and ran on headless workers (node_rank>0) that own no HTTP. Replaced with _sanity_check_external_server_args: checks tp/pp/dp/nnodes against the per-engine expectation and raises on mismatch (fail fast instead of a later rendezvous hang), gated to node_rank 0, skipping fields /server_info does not report (vLLM may omit nnodes). Note: #66 (aoshen/vllm-mirror-sglang-arch) carries the identical global-tp shadow; this is a fix both branches need, not a port. Tests: unit tests for per-engine/heterogeneous TP, the dp>1 guard, and the strict external check (match / mismatch-raises / unreported-skipped / missing-config). Existing topology unit tests are unchanged and still pass. AI assistance (Claude Code) was used for this change. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * cleanup(vllm): drop forced 0.55 gpu-mem default + redundant router fallback Remove two silent/ambiguous defaults in the vLLM launch path: - launch_server_process no longer forces --gpu-memory-utilization=0.55. In colocate, training and rollout do not occupy the GPU simultaneously (sleep/offload cycles), so vLLM's own default is appropriate; a user value via --vllm-gpu-memory-utilization is still auto-forwarded by _forward_vllm_cli_args. (This reverts the unset default to vLLM's; memory-tight large-model colocate setups can set the flag explicitly.) - VLLMEngine.init: drop the `else self.args.vllm_router_ip/port` fallback. rollout always calls engine.init(router_ip=self.router_ip, router_port=self.router_port) and _start_router always returns a real address, so the fallback was dead/redundant. - Update the arguments.py note that referenced the removed 0.55 default. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * cleanup(vllm): use _response_json helper + drop dead fields/aliases - _sanity_check_external_server_args now uses the _response_json helper (consistent error handling — annotates HTTP errors with response text) instead of a manual raise_for_status + .json(). - Remove unused VllmEngineTopology.master_host/master_port fields: never set or read (append_vllm_distributed_launch_flags takes the master addr via its own param). - Remove three test-only back-compat aliases (_append_vllm_distributed_launch_flags, _redact_cmd_for_log, _serialize_for_cli); tests now call the public names directly. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(vllm): central node_rank guard for control-plane HTTP (headless workers) Mirror SGLang's _make_request choke point: route the node_rank guard through the single shared POST helper so every control-plane method that POSTs is guarded by construction, instead of scattered (and incomplete) per-method `if node_rank != 0` checks. Before, 7 control-plane HTTP methods were unguarded (release/resume_memory_occupation, init_weight_transfer_engine, start/finish_weight_update, init_weights_update_group, update_weights_from_distributed): on a headless worker (node_rank>0, no HTTP server) they would hit a non-existent endpoint instead of no-op'ing. - `_post_json` (the central POST path, = SGLang's `_make_request`) now short-circuits to None on node_rank>0; `_response_json(None)` returns None so the no-op propagates to callers with no per-call special-casing and no return-type change (mocked tests unaffected). - `/sleep` and `/wake_up` bypass `_post_json` (query params), so they keep an explicit guard — same shape as SGLang's explicit guards on its non-_make_request methods. Tests: headless worker no-ops all control-plane methods with zero HTTP; `_post_json` short-circuits; `_response_json(None) -> None`. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(vllm): route control-plane POSTs through _make_request (SGLang-style) Add `_make_request` (guard + POST + JSON parse), mirroring SGLang's HttpServerEngineAdapter._make_request, and route the control-plane POST callers through it: _post_vllm_update_weights_http, init_weight_transfer_engine, init_weights_update_group, start_weight_update, finish_weight_update. Each becomes a one-liner (no more `response = self._post_json(...); return _response_json(response)`). The node_rank guard stays centralized in `_post_json` (which `_make_request` wraps), so behavior is unchanged and tests that mock `_post_json` are unaffected (no return-type change, no test ripple). Verified: headless workers (node_rank>0) no-op every control-plane method with zero HTTP; node-0 posts + parses normally. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(vllm): collapse _post_json into _make_request (single choke point) Per review feedback, `_make_request -> _post_json -> _response_json` was one layer too many. SGLang's `_make_request` is self-contained (guard + POST inline), so inline `_post_json` into `_make_request` (node_rank guard + POST + parse via the shared `_response_json`, which the query-param endpoints /sleep, /wake_up also reuse) and drop `_post_json` entirely. `_response_json` reverts to strict (the None-tolerance is unneeded now that nothing passes it None). Tests that mocked `_post_json` now mock `_make_request` (which returns parsed JSON), with no other behavior change. Verified: headless workers (node_rank>0) no-op every control-plane method with zero HTTP; node-0 posts + parses; `_post_json` is fully removed. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(vllm): model _MockResponse.content so the empty-200-body path is tested test_response_json_empty_body_returns_ok built _MockResponse(text="") and expected {"ok": True}, but _MockResponse had no `content` attribute while _response_json checks `response.content` — so the test raised AttributeError instead of exercising the empty-body branch (the /sleep, /wake_up empty-200 handling, cf. #80). Give _MockResponse a `content` (JSON-body bytes when json_data is set, else the text bytes, i.e. b"" when empty) so the empty-body handling is actually verified. Other _MockResponse usages set json_data and thus get non-empty content — unaffected. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(vllm): reorder vllm_engine.py functions by lifecycle (no logic change) Pure top-level reordering for readability — group module functions by stage: shared helpers -> topology -> launch config (compute_server_args) -> command/env build (build_vllm_cmd_and_env) -> process spawn (launch_server_process) -> misc -> the VLLMEngine actor. No behavior change (module-level functions resolve names at call time). Verified the module imports cleanly and the topology / control-plane (headless no-op) drivers still pass; a reorder script asserted no non-blank line was added or removed (diff is a balanced 71/71 move). Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rollout): always use the full Rust router for PD; remove the unusable MiniLB path (#88) _start_router defaulted PD to MiniLB (mini_lb=True unless SLIME_VLLM_ROUTER_USE_RUST=1). MiniLB (vllm_router/mini_lb.py) is a debug-only load balancer that REQUIRES static prefill/decode URLs at construction; slime never provides those (it launches the router first and engines register dynamically via POST /workers), so MiniLB can never work here. Remove the MiniLB activation and the dead SLIME_VLLM_ROUTER_USE_RUST gate. PD now uses the full Rust router (accepts dynamic registration like the non-PD path). MiniLB stays off via RouterArgs' own default (mini_lb=False) — no explicit setter needed. Also drop the stale MiniLB mention from the router-startup-failure error message. Note: routing-layer fix only; PD end-to-end still needs the engine-side KV transport (--kv-transfer-config) to initialize, a separate open blocker. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(vllm): update validate_args unit tests to the post-#85 per-engine-TP contract (#91) PR #85 removed the global vllm_tp_size from validate_args (TP is derived per-engine in vllm_engine._resolve_vllm_parallel_sizes) and moved the pp-divisibility check out of validate_args. But three test_arguments.py cases still asserted the old contract and now fail on feature/multi_nodes: - test_validate_args_pp1 (expected ns.vllm_tp_size == 4) - test_validate_args_pp2_dp2_derives_tp (expected ns.vllm_tp_size == 2) - test_validate_args_pp_indivisible_asserts (expected validate_args to raise) Rewrite them to the new contract: validate_args records pp/dp but sets no global TP, and no longer raises on pp-indivisibility (that check moved per-engine, covered in test_vllm_engine.py). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(vllm): wait for engine health with no time limit (SGLang-style), gated by liveness (#90) _wait_server_healthy hardcoded a 300s deadline that a large engine exceeds while still loading/compiling/capturing CUDA graphs (a 30B FP8 dp=2 colocate engine timed out at 300s though it was healthy seconds later). Match slime's SGLang backend: loop until /health 200 or — for a managed subprocess — until it dies (fail fast via process.is_alive()), with no overall deadline. Drop the timeout_s parameter entirely (cleaner); keep the per-probe timeout=3 so a single stuck socket can't wedge the loop. External mode (process is None) has no liveness signal and loops until reachable, by design (caller-managed engine). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
momo609
pushed a commit
that referenced
this pull request
Jun 8, 2026
* [Feature][1/N] Add vLLM multi-node rollout engine topology - VllmEngineTopology / compute_server_args for cross-node vLLM engines - Merge origin/main: add processed_logprobs default and weight-transfer comments - Fix _response_json for vLLM sleep/wake empty HTTP 200 body - Update unit tests for multi-node SKIPPED_DESTS and vllm_router_ip API * fix(vllm): per-engine TP + strict external config check (harden #68) (#85) * fix(vllm): derive rollout-engine TP per-engine + strict external config check Two fixes of one root cause — TP/parallel sizing read the *global* rollout_num_gpus_per_engine instead of the per-engine value — across the two engine launch paths. P2 (managed launch): validate_args unconditionally set a global args.vllm_tp_size (= global rollout_num_gpus_per_engine // pp) and _resolve_vllm_parallel_sizes preferred it, so the per-engine `tp = gpus_per_engine // pp` branch was dead in real runs. A heterogeneous per-group engine (e.g. num_gpus_per_engine=2, tp=2) thus launched with the global TP while the trainer sized the NCCL weight-transfer rendezvous from the per-group engine_gpu_counts — they disagreed and the rendezvous hung 300s ("3/4 clients joined"). TP is now derived per engine in _resolve_vllm_parallel_sizes (no global shadow), matching upstream slime's sglang_engine; the global vllm_tp_size computation is removed. dp>1 raises NotImplementedError (DP/EP wiring is a follow-up); pp divisibility validated per engine. P1 (external engine): _wait_external_config_ready compared the engine's reported TP against the same global flag and only warned, and ran on headless workers (node_rank>0) that own no HTTP. Replaced with _sanity_check_external_server_args: checks tp/pp/dp/nnodes against the per-engine expectation and raises on mismatch (fail fast instead of a later rendezvous hang), gated to node_rank 0, skipping fields /server_info does not report (vLLM may omit nnodes). Note: #66 (aoshen/vllm-mirror-sglang-arch) carries the identical global-tp shadow; this is a fix both branches need, not a port. Tests: unit tests for per-engine/heterogeneous TP, the dp>1 guard, and the strict external check (match / mismatch-raises / unreported-skipped / missing-config). Existing topology unit tests are unchanged and still pass. AI assistance (Claude Code) was used for this change. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * cleanup(vllm): drop forced 0.55 gpu-mem default + redundant router fallback Remove two silent/ambiguous defaults in the vLLM launch path: - launch_server_process no longer forces --gpu-memory-utilization=0.55. In colocate, training and rollout do not occupy the GPU simultaneously (sleep/offload cycles), so vLLM's own default is appropriate; a user value via --vllm-gpu-memory-utilization is still auto-forwarded by _forward_vllm_cli_args. (This reverts the unset default to vLLM's; memory-tight large-model colocate setups can set the flag explicitly.) - VLLMEngine.init: drop the `else self.args.vllm_router_ip/port` fallback. rollout always calls engine.init(router_ip=self.router_ip, router_port=self.router_port) and _start_router always returns a real address, so the fallback was dead/redundant. - Update the arguments.py note that referenced the removed 0.55 default. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * cleanup(vllm): use _response_json helper + drop dead fields/aliases - _sanity_check_external_server_args now uses the _response_json helper (consistent error handling — annotates HTTP errors with response text) instead of a manual raise_for_status + .json(). - Remove unused VllmEngineTopology.master_host/master_port fields: never set or read (append_vllm_distributed_launch_flags takes the master addr via its own param). - Remove three test-only back-compat aliases (_append_vllm_distributed_launch_flags, _redact_cmd_for_log, _serialize_for_cli); tests now call the public names directly. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(vllm): central node_rank guard for control-plane HTTP (headless workers) Mirror SGLang's _make_request choke point: route the node_rank guard through the single shared POST helper so every control-plane method that POSTs is guarded by construction, instead of scattered (and incomplete) per-method `if node_rank != 0` checks. Before, 7 control-plane HTTP methods were unguarded (release/resume_memory_occupation, init_weight_transfer_engine, start/finish_weight_update, init_weights_update_group, update_weights_from_distributed): on a headless worker (node_rank>0, no HTTP server) they would hit a non-existent endpoint instead of no-op'ing. - `_post_json` (the central POST path, = SGLang's `_make_request`) now short-circuits to None on node_rank>0; `_response_json(None)` returns None so the no-op propagates to callers with no per-call special-casing and no return-type change (mocked tests unaffected). - `/sleep` and `/wake_up` bypass `_post_json` (query params), so they keep an explicit guard — same shape as SGLang's explicit guards on its non-_make_request methods. Tests: headless worker no-ops all control-plane methods with zero HTTP; `_post_json` short-circuits; `_response_json(None) -> None`. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(vllm): route control-plane POSTs through _make_request (SGLang-style) Add `_make_request` (guard + POST + JSON parse), mirroring SGLang's HttpServerEngineAdapter._make_request, and route the control-plane POST callers through it: _post_vllm_update_weights_http, init_weight_transfer_engine, init_weights_update_group, start_weight_update, finish_weight_update. Each becomes a one-liner (no more `response = self._post_json(...); return _response_json(response)`). The node_rank guard stays centralized in `_post_json` (which `_make_request` wraps), so behavior is unchanged and tests that mock `_post_json` are unaffected (no return-type change, no test ripple). Verified: headless workers (node_rank>0) no-op every control-plane method with zero HTTP; node-0 posts + parses normally. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(vllm): collapse _post_json into _make_request (single choke point) Per review feedback, `_make_request -> _post_json -> _response_json` was one layer too many. SGLang's `_make_request` is self-contained (guard + POST inline), so inline `_post_json` into `_make_request` (node_rank guard + POST + parse via the shared `_response_json`, which the query-param endpoints /sleep, /wake_up also reuse) and drop `_post_json` entirely. `_response_json` reverts to strict (the None-tolerance is unneeded now that nothing passes it None). Tests that mocked `_post_json` now mock `_make_request` (which returns parsed JSON), with no other behavior change. Verified: headless workers (node_rank>0) no-op every control-plane method with zero HTTP; node-0 posts + parses; `_post_json` is fully removed. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(vllm): model _MockResponse.content so the empty-200-body path is tested test_response_json_empty_body_returns_ok built _MockResponse(text="") and expected {"ok": True}, but _MockResponse had no `content` attribute while _response_json checks `response.content` — so the test raised AttributeError instead of exercising the empty-body branch (the /sleep, /wake_up empty-200 handling, cf. #80). Give _MockResponse a `content` (JSON-body bytes when json_data is set, else the text bytes, i.e. b"" when empty) so the empty-body handling is actually verified. Other _MockResponse usages set json_data and thus get non-empty content — unaffected. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(vllm): reorder vllm_engine.py functions by lifecycle (no logic change) Pure top-level reordering for readability — group module functions by stage: shared helpers -> topology -> launch config (compute_server_args) -> command/env build (build_vllm_cmd_and_env) -> process spawn (launch_server_process) -> misc -> the VLLMEngine actor. No behavior change (module-level functions resolve names at call time). Verified the module imports cleanly and the topology / control-plane (headless no-op) drivers still pass; a reorder script asserted no non-blank line was added or removed (diff is a balanced 71/71 move). Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rollout): always use the full Rust router for PD; remove the unusable MiniLB path (#88) _start_router defaulted PD to MiniLB (mini_lb=True unless SLIME_VLLM_ROUTER_USE_RUST=1). MiniLB (vllm_router/mini_lb.py) is a debug-only load balancer that REQUIRES static prefill/decode URLs at construction; slime never provides those (it launches the router first and engines register dynamically via POST /workers), so MiniLB can never work here. Remove the MiniLB activation and the dead SLIME_VLLM_ROUTER_USE_RUST gate. PD now uses the full Rust router (accepts dynamic registration like the non-PD path). MiniLB stays off via RouterArgs' own default (mini_lb=False) — no explicit setter needed. Also drop the stale MiniLB mention from the router-startup-failure error message. Note: routing-layer fix only; PD end-to-end still needs the engine-side KV transport (--kv-transfer-config) to initialize, a separate open blocker. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(vllm): update validate_args unit tests to the post-#85 per-engine-TP contract (#91) PR #85 removed the global vllm_tp_size from validate_args (TP is derived per-engine in vllm_engine._resolve_vllm_parallel_sizes) and moved the pp-divisibility check out of validate_args. But three test_arguments.py cases still asserted the old contract and now fail on feature/multi_nodes: - test_validate_args_pp1 (expected ns.vllm_tp_size == 4) - test_validate_args_pp2_dp2_derives_tp (expected ns.vllm_tp_size == 2) - test_validate_args_pp_indivisible_asserts (expected validate_args to raise) Rewrite them to the new contract: validate_args records pp/dp but sets no global TP, and no longer raises on pp-indivisibility (that check moved per-engine, covered in test_vllm_engine.py). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(vllm): wait for engine health with no time limit (SGLang-style), gated by liveness (#90) _wait_server_healthy hardcoded a 300s deadline that a large engine exceeds while still loading/compiling/capturing CUDA graphs (a 30B FP8 dp=2 colocate engine timed out at 300s though it was healthy seconds later). Match slime's SGLang backend: loop until /health 200 or — for a managed subprocess — until it dies (fail fast via process.is_alive()), with no overall deadline. Drop the timeout_s parameter entirely (cleaner); keep the per-probe timeout=3 so a single stuck socket can't wedge the loop. External mode (process is None) has no liveness signal and loops until reachable, by design (caller-managed engine). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PR #85 ("harden #68") removed the global
vllm_tp_sizefromvalidate_args(TP is now derived per engine invllm_engine._resolve_vllm_parallel_sizes) and moved the pp-divisibility check out ofvalidate_args. Threetest_arguments.pycases still assert the old contract and therefore fail onfeature/multi_nodestoday:test_validate_args_pp1— expectedns.vllm_tp_size == 4test_validate_args_pp2_dp2_derives_tp— expectedns.vllm_tp_size == 2test_validate_args_pp_indivisible_asserts— expectedvalidate_argsto raiseFix
Rewrite them to the post-#85 contract:
validate_argsrecordspp/dpbut sets no global TP (assertvllm_tp_sizeis absent), and no longer raises on pp-indivisibility (that check moved per-engine and is covered intest_vllm_engine.py).Validation
The identical change was run in the r3 container as part of the broader PR#68 hardening work (full
tests/unit/backends/vllm_utils/suite 81/81). It is a test-only change.AI assistance (Claude Code) was used for this change.