fix vlm support asyncgrpo - #6839
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c5c2311. Configure here.
| if self._is_vlm: | ||
| model.model.requires_grad_(False) | ||
| model.model.language_model.requires_grad_(True) | ||
|
|
There was a problem hiding this comment.
Packed Qwen3.5 training leaks across samples
High Severity
Enabling Qwen3.5 / Qwen3.6 VLM training still packs several rollouts into one row and only passes 2D position_ids into the patched forward, with no cu_seqlens or other packed-boundary metadata. Those checkpoints use Gated DeltaNet (and VLMs expand M-RoPE to 3D), so linear-attention conv/recurrent state is not reset at sample boundaries. Logprobs then mix tokens from unrelated packed sequences, which breaks the GRPO ratio against vLLM’s per-sample generation.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c5c2311. Configure here.
|
|
||
| ## Vision-language models | ||
|
|
||
| Checkpoints that ship a vision tower — Qwen3.5, Qwen3.6, Qwen3-VL, … — can be trained on **text-only** datasets. Pass the model id as usual; nothing else changes: |
There was a problem hiding this comment.
"Checkpoints that ship a vision tower — Qwen3.5, Qwen3.6, Qwen3-VL, … —"
in other words: Vision language models ;)
qgallouedec
left a comment
qgallouedec
left a comment
There was a problem hiding this comment.
Thanks, I didn't test myself, but overall it looks good, just some minor non-blocking remarks
|
|
||
| Two things are worth knowing about how this works: | ||
|
|
||
| - **The whole model is loaded, not just its text tower.** vLLM serves the `*ForConditionalGeneration` architecture, whose parameters are named `model.language_model.*` and `model.visual.*`. Loading only the text tower would name them `model.*` and every NCCL weight transfer would fail on unknown keys. So the vision tower is loaded too, and costs GPU memory for its weights. |
There was a problem hiding this comment.
Loading only the text tower would name them
model.*and every NCCL weight transfer would fail on unknown keys. So the vision tower is loaded too, and costs GPU memory for its weights.
I think this is an internal detail, it's not worth explaining in the doc. But NIT
| Two things are worth knowing about how this works: | ||
|
|
||
| - **The whole model is loaded, not just its text tower.** vLLM serves the `*ForConditionalGeneration` architecture, whose parameters are named `model.language_model.*` and `model.visual.*`. Loading only the text tower would name them `model.*` and every NCCL weight transfer would fail on unknown keys. So the vision tower is loaded too, and costs GPU memory for its weights. | ||
| - **The vision tower is frozen.** A text-only dataset never produces image tokens, so the tower is never exercised by the forward pass. Everything outside the text tower (vision tower, multimodal projector) has `requires_grad=False`: it gets no gradients and no optimizer state, and weight sync skips it entirely — the server keeps the values it loaded from the checkpoint. |
|
|
||
| def test_vision_tower_is_frozen(self, model_id): | ||
| trainer = self._trainer(model_id) | ||
| assert trainer._is_vlm |
There was a problem hiding this comment.
| assert trainer._is_vlm |
let's not test on private vars.
| def test_train(self, model_id): | ||
| trainer = self._trainer(model_id) | ||
| previous_params = {n: param.clone() for n, param in trainer.model.named_parameters()} | ||
|
|
||
| trainer.train() | ||
|
|
||
| assert trainer.state.log_history[-1]["train_loss"] is not None | ||
|
|
||
| for n, param in previous_params.items(): | ||
| new_param = trainer.model.get_parameter(n) | ||
| if new_param.requires_grad: | ||
| assert not torch.equal(param, new_param), f"Parameter {n} has not changed." | ||
| else: | ||
| assert torch.equal(param, new_param), f"Frozen parameter {n} has changed." |
There was a problem hiding this comment.
If there is a simple way to do this, I think it would be better to branch on whether the param in in the vision tower or not.
| self._is_vlm = text_config is not model.config | ||
| if self._is_vlm: | ||
| model.model.requires_grad_(False) | ||
| model.model.language_model.requires_grad_(True) |
There was a problem hiding this comment.
for ref, non-blocking, this might not work in general, for ref, this is another approach https://github.com/huggingface/trl/pull/6618/changes#diff-5f63a484ac9f57663f1e8d18ab41afe1d55e7909133191175cbc4943197f39adR132-R146
2 participants
What does this PR do?
Scoped down from #6515 on purpose
Adds support to VLMs for
AsyncGRPOTrainer, this DOESNT support multimodal training.create_model_from_pathcompute_flops_per_tokenandpatch_chunked_lm_head_checkpoint_wrapped_module.from synced weight names, matching sync GRPO's_fix_param_name_to_vllm.cc @adithya-s-k @qgallouedec
Note
Cursor Bugbot is generating a summary for commit c5c2311. Configure here.