Sync to vLLM 0.21.0 - #21
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the vLLM weight transfer mechanism by replacing the subprocess-based _NcclBridge with an in-process implementation using NCCLWeightTransferEngine. It introduces session management for weight updates and adds support for new HTTP endpoints in the VLLMEngine. Feedback was provided regarding the _response_json_or_fallback helper, noting that hardcoding a success flag in fallback paths could mask server errors or malformed responses.
| def _response_json_or_fallback(response: requests.Response) -> dict: | ||
| """Parse JSON body; on decode failure return a minimal dict with raw text.""" | ||
| try: | ||
| body = response.json() | ||
| if isinstance(body, dict): | ||
| return body | ||
| return {"ok": True, "data": body} | ||
| except ValueError: | ||
| return {"ok": True, "raw": response.text} |
There was a problem hiding this comment.
The _response_json_or_fallback helper hardcodes "ok": True in all fallback paths, including when JSON decoding fails or when the response body is not a dictionary. This is misleading because it masks potential logical errors or malformed responses from the vLLM server that didn't result in a non-2xx HTTP status code. If the server returns plain text indicating an error but with a 200 OK status, this function will report success to the caller.
| def _response_json_or_fallback(response: requests.Response) -> dict: | |
| """Parse JSON body; on decode failure return a minimal dict with raw text.""" | |
| try: | |
| body = response.json() | |
| if isinstance(body, dict): | |
| return body | |
| return {"ok": True, "data": body} | |
| except ValueError: | |
| return {"ok": True, "raw": response.text} | |
| def _response_json_or_fallback(response: requests.Response) -> dict: | |
| """Parse JSON body; on decode failure return a minimal dict with raw text.""" | |
| try: | |
| body = response.json() | |
| if isinstance(body, dict): | |
| return body | |
| return {"ok": False, "error": "Response is not a dictionary", "data": body} | |
| except ValueError: | |
| return {"ok": False, "error": "Invalid JSON response", "raw": response.text} |
9d8b56f to
89c9f02
Compare
|
/gemini review this |
There was a problem hiding this comment.
Code Review
This pull request refactors the vLLM weight transfer mechanism to use an in-process NCCLWeightTransferEngine, replacing the previous subprocess-based _NcclBridge. It introduces explicit session management with start_weight_update and finish_weight_update calls and updates the VLLMEngine to support these new endpoints. Feedback suggests optimizing performance by moving local imports and caching environment variable lookups that occur within hot loops.
| group.send_weights_packed(list(converted_named_tensors)) | ||
| else: | ||
| group.broadcast_tensors([param.data for _, param in converted_named_tensors]) | ||
| from vllm.distributed.weight_transfer.nccl_engine import NCCLTrainerSendWeightsArgs, NCCLWeightTransferEngine |
There was a problem hiding this comment.
The local imports for NCCLTrainerSendWeightsArgs and NCCLWeightTransferEngine are performed inside update_weights_from_distributed, which is called for every bucket of weights during the sync process. While sys.modules caching makes this relatively fast, performing these lookups in a hot loop adds unnecessary overhead. Consider moving these imports to the top of the file (using a try-except block if vllm is an optional dependency) or caching them at the module level.
| def _weight_transfer_http_timeout(self) -> float: | ||
| return float( | ||
| os.environ.get( | ||
| "SLIME_VLLM_WEIGHT_TRANSFER_UPDATE_TIMEOUT_SEC", | ||
| os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900"), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The _weight_transfer_http_timeout method performs multiple os.environ.get calls every time it is invoked. Since this is used in _post_vllm_update_weights_http, which is called per weight bucket, this results in redundant environment variable lookups. It is more efficient to resolve and cache this timeout value once during __init__ or the init method.
| def _weight_transfer_http_timeout(self) -> float: | |
| return float( | |
| os.environ.get( | |
| "SLIME_VLLM_WEIGHT_TRANSFER_UPDATE_TIMEOUT_SEC", | |
| os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900"), | |
| ) | |
| ) | |
| def _weight_transfer_http_timeout(self) -> float: | |
| if not hasattr(self, "_cached_timeout"): | |
| self._cached_timeout = float( | |
| os.environ.get( | |
| "SLIME_VLLM_WEIGHT_TRANSFER_UPDATE_TIMEOUT_SEC", | |
| os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900"), | |
| ) | |
| ) | |
| return self._cached_timeout |
| named_gpu_iter, | ||
| NCCLTrainerSendWeightsArgs(group=group, packed=packed), | ||
| ) | ||
| torch.cuda.synchronize() |
There was a problem hiding this comment.
What about syncing after all weight bucket are sent?
|
Please refactor this in slime/backends/vllm_utils/arguments.py#L220: |
| group.send_weights_packed(list(converted_named_tensors)) | ||
| else: | ||
| group.broadcast_tensors([param.data for _, param in converted_named_tensors]) | ||
| from vllm.distributed.weight_transfer.nccl_engine import NCCLTrainerSendWeightsArgs, NCCLWeightTransferEngine |
* MegatronvLLM native sync: in-process NCCL weight transfer (remove NcclBridge) * create unit tests file and update vllm_engine format * address PR #21 review: NCCL imports, sync placement, and HTTP helpers
Two regressions surfaced after PR #18 merged PR #22 (update_weights_tensor) into gcl/clean-sglang: 1. ``slime/backends/vllm_utils/vllm_engine.py`` had duplicate ``def start_weight_update`` / ``def finish_weight_update`` definitions. PR #21 (#f7ac630, "Sync to vLLM 0.21.0") added a leader-aware version at ~line 569/581 with ``_skipped_if_not_leader()`` self-protection. PR #22's #47158c1 commit added its own no-skip version at ~line 820/834 without removing PR #21's. Python class-body semantics shadow the earlier definitions, so the no-skip version was the one running at runtime. The merge artifact didn't cause runtime crashes (vime engines always have ``self.node_rank = 0`` — only assignment in the codebase, line 459), but it left two defensive code paths dead-coded and confused review. Resolution: keep PR #22's design (drop leader-skip path entirely since ``node_rank`` is structurally 0 in current vime), remove the duplicates, and also remove the now-orphan ``_SKIP_NON_LEADER`` constant and ``_skipped_if_not_leader`` method. PR #22 author has been pinged (#22 comment) to also drop the corresponding ``test_*_skipped_when_node_rank_nonzero`` tests on their branch. 2. ``tests/test_update_weight_from_tensor.py::_make_instance`` (added on gcl/pr18-tests-ci via #b6a357a) bypasses ``__init__`` with ``object.__new__`` and hand-sets attributes. It was missing ``obj._ipc_engine = None`` and 12 tests that mutate ``obj._colocated_engines = [...]`` after ``_make_instance`` returned never wired ``_ipc_engine`` either. Production code at ``update_weight_from_tensor.py:237`` gates IPC lifecycle on ``self._ipc_engine is not None`` and the helper sets ``_ipc_engine`` to one designated engine in ``connect_rollout_engines`` — the tests need to replicate that designation. Resolution: add ``obj._ipc_engine = None`` to the ``_make_instance`` base, plus ``obj._ipc_engine = obj._colocated_engines[0] if obj._colocated_engines else None`` at every non-empty ``_colocated_engines`` mutation site (12 occurrences, batch-applied). Updated ``test_multiple_colocated_engines_all_get_lifecycle_calls`` to match production: release/resume/init are fanned out to all engines, but start/finish_weight_update is sent only to the designated ``_ipc_engine`` (PR #22's coordinator-per-rank design). Verified on h200-1 with vime-vllm-cu129-latest image (single GPU, Qwen3-4B compatible): - ``pytest tests/test_update_weight_from_tensor.py``: 29 passed - ``pytest tests/unit/backends/vllm_utils/test_vllm_engine.py``: 26 passed, 2 failed (``test_*_skipped_when_node_rank_nonzero`` — owner-pinged via #22). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit 3b12c77 on this branch consolidated start_weight_update / finish_weight_update to PR #22's no-leader-skip version (dropping the PR #21 defensive guard that vime's node_rank=0 architecture never exercises). The two tests below were left behind asserting the now- removed self-skip behavior: - test_start_weight_update_skipped_when_node_rank_nonzero - test_finish_weight_update_skipped_when_node_rank_nonzero Both set node_rank=1 and assert _post_json is never called; after the 3b12c77 dedup, production posts unconditionally (engines are always single-node-per-actor, node_rank is structurally 0), so these tests fail with "should not POST". Their assertion no longer reflects the production contract. Deleting them. The remaining 26 tests in this file still verify the HTTP shapes for /start_weight_update, /finish_weight_update, /update_weights, /sleep, /wake_up, etc. PR #22 author was pinged about this in #22 (comment) but has not acted; doing the cleanup on PR #18 since these tests are unblocking the unit-all batch run. After this commit: pytest tests/unit/backends/vllm_utils/test_vllm_engine.py → 26 passed (was 26 passed, 2 failed) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>





Purpose
This PR adapts vime’s Megatron→vLLM native weight sync to vLLM 0.21.x, as part of the image / dependency upgrade tracked in Issue #20 (vllm 0.21 + torch 2.11 stack).
vLLM 0.21 exposes a four-phase weight transfer control plane (vLLM #39212):
POST /start_weight_updatePOST /update_weightsPOST /finish_weight_updateTrainer-side sends use
NCCLTrainerSendWeightsArgswithNCCLWeightTransferEngine.trainer_send_weights(...).In-process NCCL on the trainer (replacing the subprocess bridge) is already covered by #13; this PR builds on that and focuses on 0.21 HTTP/API alignment plus unit test coverage.
What’s included
vLLM 0.21 weight sync (
update_weight_from_distributed.py)start_weight_update(is_checkpoint_format=True)→ per-bucket/update_weights+trainer_send_weights(NCCLTrainerSendWeightsArgs(...))→finish_weight_update, with Gloo barriers.update_infono longer carries legacyis_checkpoint_format(handled instart).VLLMEngine(vllm_engine.py)_response_json_or_fallback(), unified weight-transfer HTTP timeout,_http_base()init guard.Unit tests (new layout)
Introduce layered unit tests under
tests/unit/(pytestunitmarker unchanged):tests/unit/backends/vllm_utils/test_vllm_engine.py_normalize_vllm_wake_tags,_serialize_for_cli, init retry, IPv6 base URL, etc.) — 28 cases, no live vLLM servertests/unit/backends/vllm_utils/test_arguments.pytests/test_vllm_arguments.py(CLI / router args)tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_distributed.pytests/test_update_weight_from_distributed.py; covers in-process NCCL send, start/finish session, packed bucketstests/unit/conftest.pyRun:
pytest tests/unit/backends -m unit -v # or pytest tests/unit/backends/vllm_utils/test_vllm_engine.py -m unit -vTest plan
pytest tests/unit/backends -m unit -vbash run_scripts/qwen_4b.shwith--rollout-backend vllm --vllm-weight-sync-mode native/start_weight_update,/update_weights,/finish_weight_update(0.21+)Environment / install: same as #3 and #20 (vllm 0.21 cu129 image work in #17).
Test result
test_vllm_engine.py: 28 passed (HTTP mocks)test_update_weight_from_distributed.py: passed on training host (incl. start/finish session + Gloo mock)Known issues / follow-up
NCCLTrainerSendWeightsArgs(see [RFC] Docker image roadmap #20).slime_vllm_backend_design_v1.mdstill mentions NcclBridge — update separately.