code to patch for qwen3.5 grpo training - #10957
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Qwen3.5-VL models, featuring standalone position ID computation and updated dataset processing. It also includes extensive compatibility fixes for newer versions of vLLM (0.17+) and transformers. A key feature added is the manual merging of LoRA weights into the base model on the CPU to support hybrid architectures. Review feedback highlights critical memory efficiency concerns when gathering weights for large models, potential non-determinism in distributed ID generation, and a possible crash in the vision position ID logic.
| for key in list(actor_weights.keys()): | ||
| v = actor_weights[key] | ||
| if isinstance(v, DTensor): | ||
| actor_weights[key] = v.full_tensor().detach().cpu() | ||
| else: | ||
| actor_weights[key] = v.detach().cpu() | ||
| actor_weights = self._merge_lora_into_weights(actor_weights, lora_weights) |
There was a problem hiding this comment.
This block gathers all model weights into CPU memory simultaneously by calling full_tensor() on every parameter. For large models (e.g., 70B+), this will likely lead to CPU OOM, especially since this happens on every rank. The existing _make_weight_iterator (line 99) avoids this by gathering and yielding tensors one by one. You should refactor the LoRA merging logic to perform the merge lazily during iteration, or at least gather-merge-release each tensor individually to maintain memory efficiency.
| if not self.is_lora or self.merge_lora_for_rollout: | ||
| model.load_weights(self._make_weight_iterator(actor_weights)) | ||
| else: | ||
| lora_int_id = int(time.time_ns() % 0x7FFFFFFF) |
There was a problem hiding this comment.
Generating lora_int_id using time.time_ns() is non-deterministic across distributed processes. In a multi-GPU setup where multiple ranks share a vLLM engine (e.g., when tensor_parallel_size > 1), different ranks might generate different IDs, causing vLLM to fail or behave inconsistently. This ID should be synchronized across ranks (e.g., using dist.broadcast) or derived from a deterministic source.
| ) | ||
| current_pos += text_len | ||
| else: # image (1) or video (2) | ||
| grid_thw = next(grid_iters[modality_type]) |
There was a problem hiding this comment.
The code assumes that grid_iters[modality_type] is an iterator. However, if image_grid_thw or video_grid_thw is None, the corresponding entry in grid_iters is initialized to None (see lines 110-111). Calling next(None) will raise a TypeError. You should check if the iterator exists before calling next or ensure that the input tensors are provided if the modality is present in mm_token_type_ids.
| weight_key = f"{model_key}.weight" | ||
| if weight_key in actor_weights: | ||
| delta = (pair["b"] @ pair["a"]) * scaling | ||
| actor_weights[weight_key] = actor_weights[weight_key] + delta.to(actor_weights[weight_key].dtype) |
There was a problem hiding this comment.
Using actor_weights[weight_key] + delta creates a new tensor copy. For large models, this can significantly increase CPU memory usage during the merge process. Consider using the in-place add_ method instead.
| actor_weights[weight_key] = actor_weights[weight_key] + delta.to(actor_weights[weight_key].dtype) | |
| actor_weights[weight_key].add_(delta.to(actor_weights[weight_key].dtype)) |
|
good job for supporting qwen3.5, however when use this patch, error still occure saying transformers need to upgrade to support qwen3.5. Then more problem happen after update transformer, seems that patched qwen3.5 model not work.Please help detail how to use the patch. |
Hi, this is just for my experiment, I have a script to build an image, I also have another 2 local patch script to patch vllm. |
Hey thanks for sharing the amazing work, but are there any new progress toward this one then |
3 participants
No description provided.