feat(rl): multimodal context parallelism#2948
Merged
Merged
Conversation
This was referenced Jul 3, 2026
61959a4 to
b0e3d09
Compare
8b1fc96 to
027edc2
Compare
b0e3d09 to
f1920ce
Compare
This was referenced Jul 3, 2026
f1920ce to
f34c10d
Compare
496a1b5 to
9c6eb77
Compare
f34c10d to
ce84dbf
Compare
b99a705 to
d8ce43b
Compare
ce84dbf to
1c096cf
Compare
1c096cf to
7b18d58
Compare
d8ce43b to
1b600e5
Compare
7b18d58 to
7c38ffa
Compare
1b600e5 to
0221756
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7c38ffa. Configure here.
7c38ffa to
dd21849
Compare
0221756 to
ab8ac2d
Compare
dd21849 to
3e99cac
Compare
3527041 to
7edc462
Compare
Mirror the SFT VLM CP path in the RL trainer, replacing the blanket NotImplementedError: - MRoPE multimodal batches under ulysses defer sharding to the model: input_ids/position_ids stay global so the vision encoder and image-embed merge see the full sequence; the model root forward shards embeds/positions after merge (and routed_experts with them). Text batches keep the existing pre-shard path. - seq_lens passes with seq_lens_are_global=True under CP — micro-batch boundaries span the pre-shard sequence and pad_to_multiple_of=cp already guarantees shard divisibility (padding is its own seq_lens segment). - The LoRA token-count CP adjustment derives chunk_size from the sharded length even when input_ids stays global. - Config gate relaxed: VLM + CP now requires cp_style='ulysses' and a custom model implementation instead of being rejected outright; resolve_pack_multimodal drops its CP rejection (packed MM rows meet the same divisibility guarantee). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Use the pre-shard boundary API, rely on shared CP implementation validation, and derive local LoRA chunk sizes from already-sharded labels. Co-authored-by: Cursor <cursoragent@cursor.com>
3e99cac to
bd09c63
Compare
bd09c63
into
feat/rl-multimodal-packing
4 of 15 checks passed
Contributor
Author
|
Folded into #2947 now that the RL multimodal CP delta is small and directly coupled to multimodal packing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Part 7/7 — the only new code in the stack (in neither #2485 nor #2889; both deferred RL MM CP). Based on #2947 (
feat/rl-multimodal-packing) — needs MM packing from #2947, the VLM CP machinery from #2946 (which is trainer-agnostic, in the modeling files), and the seq_lens contract from #2944. Rebased onto the current #2947 after its packing review fixes and the finalized #2946 CP cleanup.Mirrors the SFT VLM CP path in the RL trainer, replacing the blanket
NotImplementedError:input_ids/position_idsstay global so the vision encoder and image-embed merge see the full sequence; the model root forward shards embeds/positions after merge (androuted_expertswith them). Text batches keep the existing pre-shard path.seq_lenspasses withseq_lens_are_pre_shard=Trueunder CP — micro-batch boundaries span the pre-shard sequence, andpad_to_multiple_of=cpalready guarantees shard divisibility (padding is its ownseq_lenssegment since feat(trainer): seq_lens as the packed-sample boundary contract (SFT + RL) #2944).chunk_sizefrom the sharded length even wheninput_idsstays global.cp_style='ulysses'and a custom model implementation instead of blanket rejection (impl="auto"passes the validator;get_modelrejects it at load time if it resolves to hf).Proposed follow-up (not in this stack): the deferral condition and sharding choreography here deliberately mirror
sft/train.py; extracting the shared logic into one helper would keep the two trainer loops from drifting.E2E
RL MM cp=2 ulysses run (
multimodal_color_codewordshape) — W&B link TBDIntegration suite on 8×H100 at this tip: 27 passed, 6 failed, 1 error — every failure also fails (or is family-flaky) on unmodified
mainon the same node: 4×test_benchmark_regression(tolerances calibrated for the CI A6000 runners),test_reverse_text_moe(FileNotFoundError, env),test_reverse_text_multi_run(flaky family — main fails a sibling test). The one stack-correlated flip,test_reverse_text_sft::test_loss_goes_down, is a marginal assertion: 5 steps at batch 4, last-vs-first loss delta +0.0018, and this branch's loss/grad values match the feat(sft): Enable VLM SFT #2485-head reference bit-for-bit (5.5987/73.2068). Suggest lengthening that CI run or comparing first-k/last-k means — flagged for review.🤖 Generated with Claude Code
Note
Medium Risk
Touches distributed RL forward/CP sharding for multimodal batches; incorrect boundary or shard handling could skew logprobs/gradients, but scope is aligned with existing SFT behavior and gated to ulysses + custom models.
Overview
RL multimodal training can use context parallelism when
cp_style='ulysses', instead of being rejected outright.The RL trainer mirrors the SFT VLM CP path: for MRoPE batches (multimodal kwargs with
image_grid_thw), it skips pre-forward sharding ofinput_ids/position_idsso vision merge and image embeds see the full sequence; the custom model shards after merge. Text-only CP batches still use the existingsetup_cp_paramspath.seq_lensstays on the wire under CP withseq_lens_are_pre_shard=Trueso packed document boundaries refer to the pre-shard sequence. LoRA CP token counting uses shardedlabelslength forchunk_sizewhen inputs stay global.Config validation replaces blanket “VLM + CP forbidden” with VLM + CP requires
cp_style='ulysses'(ring CP with VLM still errors).Docs add a development guide for custom VLM implementations (registration, packed
seq_lens, dummy vision for FSDP, optional CP hooks).Tests swap the old rejection test for ring-vs-ulysses coverage and assert ulysses + VLM +
cp=2validates.Reviewed by Cursor Bugbot for commit bd09c63. Bugbot is set up for automated code reviews on this repo. Configure here.