fix: vLLM 0.21 colocate IPC weight sync - #23
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a weight synchronization mechanism for colocated vLLM engines using CUDA IPC via Ray, introducing the UpdateVLLMWeightFromTensor class and a vLLM worker extension. Key feedback includes identifying a logic error in the server launch process that could skip the sleep mode flag for multiple engines and pointing out redundant lifecycle calls that should be restricted to rank 0. Additionally, the reviewer suggested moving a synchronization barrier outside the weight chunk loop to improve performance and recommended against setting global environment variables within the class constructor.
| if (getattr(args, "offload_rollout", False) or getattr(args, "colocate", False)) and not getattr( | ||
| args, "vllm_enable_sleep_mode", False | ||
| ): | ||
| cmd += ["--enable-sleep-mode"] | ||
| args.vllm_enable_sleep_mode = True |
There was a problem hiding this comment.
There is a logic error here: if multiple engines are launched, the first one will set args.vllm_enable_sleep_mode = True. Subsequent calls to launch_server_process will then skip this block because of the and not getattr(args, "vllm_enable_sleep_mode", False) condition, resulting in subsequent vLLM servers being launched without the --enable-sleep-mode flag. The flag should be added to the command line for every engine if the condition is met, regardless of whether it was already set on the args object.
| if (getattr(args, "offload_rollout", False) or getattr(args, "colocate", False)) and not getattr( | |
| args, "vllm_enable_sleep_mode", False | |
| ): | |
| cmd += ["--enable-sleep-mode"] | |
| args.vllm_enable_sleep_mode = True | |
| if getattr(args, "offload_rollout", False) or getattr(args, "colocate", False) or getattr(args, "vllm_enable_sleep_mode", False): | |
| if "--enable-sleep-mode" not in cmd: | |
| cmd += ["--enable-sleep-mode"] | |
| args.vllm_enable_sleep_mode = True |
| # engines (not per update call). | ||
| self._ipc_initialized: bool = False | ||
| # vLLM IPC handle payloads may use cloudpickle on the Ray/HTTP bridge. | ||
| os.environ.setdefault("VLLM_ALLOW_INSECURE_SERIALIZATION", "1") |
There was a problem hiding this comment.
Setting global environment variables like VLLM_ALLOW_INSECURE_SERIALIZATION inside a class constructor is generally discouraged as it can have unintended side effects on other parts of the application or other tests in the same process. It would be better to set this at the application entry point or ensure it is only set when strictly necessary for the vLLM server process.
| if self._ipc_engine is not None: | ||
| ray.get(self._ipc_engine.start_weight_update.remote(is_checkpoint_format=True)) | ||
| dist.barrier(group=get_gloo_group()) |
There was a problem hiding this comment.
This lifecycle call is redundant when multiple trainer ranks share the same vLLM engine (e.g., in configurations where rollout_num_gpus_per_engine > 1). Since this triggers an HTTP request to the vLLM server, calling it from every rank is inefficient and could lead to race conditions or unnecessary overhead. It should be called once per engine, ideally by rank 0 for all colocated engines.
| if self._ipc_engine is not None: | |
| ray.get(self._ipc_engine.start_weight_update.remote(is_checkpoint_format=True)) | |
| dist.barrier(group=get_gloo_group()) | |
| if rank == 0 and self._colocated_engines: | |
| ray.get([engine.start_weight_update.remote(is_checkpoint_format=True) for engine in self._colocated_engines]) |
|
|
||
| # Megatron ranks can finish HF chunks at different times; sync before any | ||
| # rank calls finish_weight_update so no engine still receives update_weights. | ||
| dist.barrier(group=get_gloo_group()) |
There was a problem hiding this comment.
This barrier is inside the loop over weight chunks. If the model has many chunks, this will introduce significant synchronization overhead. Since the goal is to ensure all ranks have finished sending weights before signaling the engine to exit update mode, this barrier should be moved outside and after the loop.
| if self._ipc_engine is not None: | ||
| ray.get(self._ipc_engine.finish_weight_update.remote()) | ||
| dist.barrier(group=get_gloo_group()) |
There was a problem hiding this comment.
Similar to start_weight_update, calling finish_weight_update from every rank is redundant and inefficient. It should be called once per engine by rank 0.
| if self._ipc_engine is not None: | |
| ray.get(self._ipc_engine.finish_weight_update.remote()) | |
| dist.barrier(group=get_gloo_group()) | |
| if rank == 0 and self._colocated_engines: | |
| ray.get([engine.finish_weight_update.remote() for engine in self._colocated_engines]) |
|
Test script: |
knlnguyen1802
left a comment
There was a problem hiding this comment.
LGTM, leave some comment about refactor
| @@ -0,0 +1,355 @@ | |||
| """ | |||
There was a problem hiding this comment.
I think we can completely replace this with update_weight_from_tensor.py
| monkey_patch_torch_reductions() | ||
|
|
||
|
|
||
| class UpdateVLLMWeightFromTensor: |
There was a problem hiding this comment.
Can rename it as UpdateWeightFromTensor and remove the old one
6b19318 to
9253015
Compare
Summary
UpdateVLLMWeightFromTensorfor colocated CUDA IPC weight sync (per-enginellm_handle, per-chunk barrier).--colocate; point--worker-extension-clsat merged worker extension.Stacked on [feat] Support Colocated Weight Sync via CUDA IPC for vime #22 (
update_weights_tensor).Test plan
pytest tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_tensor_vllm.pyqwen_0.6b_colocate.shmulti-step trainingstep6: