fix(test): update vllm_generate_endpoint to post-#68 launch_server_process API - #149
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors tests/test_vllm_generate_endpoint.py to use _compute_server_args for constructing server arguments instead of the removed _visible_devices helper, aligning with a multi-node topology refactor. Feedback points out that this change can cause CUDA initialization failures in multi-GPU environments when CUDA_VISIBLE_DEVICES contains non-consecutive or non-zero indices, and suggests explicitly overriding server_args["visible_devices"] to restore the correct physical GPU mapping.
| server_args = _compute_server_args( | ||
| args, | ||
| rank=0, | ||
| visible_devices=_visible_devices(case.num_gpus), | ||
| model_path=case.model_path, | ||
| dist_init_addr=None, | ||
| host="127.0.0.1", | ||
| port=server_port, | ||
| base_gpu_id=0, | ||
| ) | ||
| process = launch_server_process(server_args) |
There was a problem hiding this comment.
By removing the _visible_devices helper and relying solely on _compute_server_args, the test inherits a limitation in _compute_server_args (specifically in _to_local_gpu_id). When CUDA_VISIBLE_DEVICES is set to non-zero/non-consecutive indices (e.g., "2,3" in a multi-GPU or containerized environment), _to_local_gpu_id maps the GPU ID to a local index (e.g., 0), resulting in server_args["visible_devices"] being set to "0". When the vllm serve subprocess is launched, its CUDA_VISIBLE_DEVICES is set to "0", causing it to attempt to run on physical GPU 0 instead of the allocated physical GPU 2. This can lead to CUDA initialization failures or resource conflicts in multi-GPU environments. To prevent this, we should explicitly override server_args["visible_devices"] with the correct physical GPU IDs from CUDA_VISIBLE_DEVICES.
server_args = _compute_server_args(
args,
rank=0,
dist_init_addr=None,
host="127.0.0.1",
port=server_port,
base_gpu_id=0,
)
cvd = os.environ.get("CUDA_VISIBLE_DEVICES")
if cvd:
server_args["visible_devices"] = ",".join(cvd.split(",")[:case.num_gpus])
else:
server_args["visible_devices"] = ",".join(str(i) for i in range(case.num_gpus))
process = launch_server_process(server_args)…r_process API `test_vllm_generate_endpoint.py` still called the pre-#68 multi-kwarg `launch_server_process(bind_host=, server_port=, args=, rank=, visible_devices=, model_path=)` form. PR #68 (multi-node rollout topology) refactored `launch_server_process` to take a single `server_args` dict built by `_compute_server_args(...)`, so the test raised at runtime: TypeError: launch_server_process() got an unexpected keyword argument 'bind_host' i.e. the test has been failing since #68 and never caught (the unit/e2e job is gated behind pre-commit, which currently fails on every PR). Fix the test to mirror how `VLLMEngine` itself launches the server: build a `server_args` dict via `_compute_server_args(args, rank=0, dist_init_addr=None, host, port)` then call `launch_server_process(server_args)`. The `args` Namespace is expanded with the attrs that `_compute_server_args` / `build_vllm_cmd_and_env` / `get_base_gpu_id` read (`num_gpus_per_node`, `hf_checkpoint`, `vllm_enable_sleep_mode`, `vllm_dp_size`, and a single-colocate placement: `colocate`, `actor_num_nodes`, `actor_num_gpus_per_node`, `use_critic`, `debug_rollout_only`). This lets the GPU base be derived through the real `get_base_gpu_id()`/`_to_local_gpu_id()` path — identical to `VLLMEngine` — so the launched server tracks `CUDA_VISIBLE_DEVICES` rather than a hardcoded base id. Drop the now-unused `_visible_devices` helper. No production code change. Verified: `test_qwen3_0_6b_vllm_inference_generate_endpoint` passes on a single GPU (1 passed, ~100s, Qwen3-0.6B, real vLLM server + /inference/v1/generate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
becc634 to
1d4f77a
Compare
Problem
tests/test_vllm_generate_endpoint.pyraised at runtime:The test still called the pre-#68 multi-kwarg form
launch_server_process(bind_host=, server_port=, args=, rank=, visible_devices=, model_path=).PR #68 (multi-node rollout topology) refactored
launch_server_processto take asingle
server_argsdict produced by_compute_server_args(...)— seeVLLMEngineitself atvime/backends/vllm_utils/vllm_engine.py(_compute_server_args(...)→launch_server_process(self._server_args)).So this test has been failing since #68 and went unnoticed: the "Unit & utils"
job is gated behind
pre-commit, which currently fails on every PR, so the jobis skipped. (Surfaced while running the full CI matrix for #121 on gb200; the
failure is not caused by #121 — the
bind_hostline predates it, from #16.)Fix
Test-only change — mirror how
VLLMEnginelaunches the server:server_argsvia_compute_server_args(args, rank=0, dist_init_addr=None, host="127.0.0.1", port=server_port, base_gpu_id=0), thenlaunch_server_process(server_args).base_gpu_id=0pins this single-engine rank-0 server to the first visible GPU, sidestepping the rollout-placement math inget_base_gpu_id(which needs a full actor/critic placement Namespace).argsNamespace with the attrs_compute_server_args/build_vllm_cmd_and_envread (num_gpus_per_node,hf_checkpoint,vllm_enable_sleep_mode,vllm_dp_size)._visible_deviceshelper.No production code change (deliberately not re-adding
bind_hosttolaunch_server_process, which would regress the #68 dict API).Verification
test_qwen3_0_6b_vllm_inference_generate_endpointnow passes on a single GPU:(Qwen3-0.6B, real
vllm servesubprocess +/inference/v1/generaterollout path, arm64 sync1916 image.)🤖 Generated with Claude Code