feat(grpo): deduplicate multimodal payloads - #3527
Conversation
…mbing and chat_template parity Folds three WIP commits (b16fca2 + 5486b57 + cd349d0): - Add tokenizer_config field to NemoGymConfig and processor reconstruction inside the actor for multi-turn multimodal postprocessing. - Add multimodal utilities (encode_images_in_examples, extract_multimodal_model_inputs, process_multimodal_chat, resolve_to_image) and their consumers in NemoGym. - Add sync single-step polish across nemo_gym.py / multimodal_utils.py / processors.py. - Maintain chat_template kwargs parity between the async vLLM worker and HF. Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit 3215893) Signed-off-by: rohitrango <rohit.rango@gmail.com>
…ntroller pathway Reword of aa04447 (async plumbing). Adds the wiring in grpo.py and run_multimodal_grpo_nemo_gym.py so that multimodal NeMo-Gym rollouts flow through the async single-controller path introduced in the multimodal foundation commit. Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit c755a25) Signed-off-by: rohitrango <rohit.rango@gmail.com>
…ygon-naming + multi-turn tool calling) Folds four WIP commits (28ba38e + 81f885f + ecf72e4 + 7171787): - Bump seq len and generation count on the doorkey smoke config. - Add tangram env smoke config and its launcher. - Add multi-turn multimodal tool calling / polygon-naming smoke config and launcher. Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit b54e6a8) Signed-off-by: rohitrango <rohit.rango@gmail.com>
…m_to_nemo_rl_result Commit ec8333d added `nemo_gym_row: dict` as a new positional parameter to `NemoGym._postprocess_nemo_gym_to_nemo_rl_result` but never referenced it in the function body, and neither the sole in-tree caller (`_run_rollouts_iterator` in the same file) nor the six unit-test call sites (`tests/unit/environments/test_nemo_gym{,_router_replay}.py`) were updated to pass it. As a result every sync/async multimodal smoke recipe under `examples/nemo_gym/run_gymv_smoke.sh` crashed with TypeError: NemoGym._postprocess_nemo_gym_to_nemo_rl_result() missing 1 required positional argument: 'tokenizer' as soon as the first rollout came back from the Gym HTTP server — the `nemo_gym_result` positional was being consumed by the phantom `nemo_gym_row` slot, so `tokenizer` looked missing. Since the parameter is unused, the minimal fix is to remove it and restore the original 2-arg `(nemo_gym_result, tokenizer)` signature. This lines back up with all six test call sites (which were already passing 2 args) and the production caller at nemo_gym.py:442 (which was already passing 2 args), so no other files need touching. Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit a2f8fd8) Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit 7ddf223) Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit 7bf55af) Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit b7fc746) Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
except assistant) Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit 85a24b8) Signed-off-by: rohitrango <rohit.rango@gmail.com>
`_index_per_turn_images` gated its bucket flush on `role == "assistant"`, but `_postprocess_nemo_gym_to_nemo_rl_result` treats every item carrying truthy `generation_token_ids` as a trainable turn — including reasoning-only responses and `function_call` items whose role is not `"assistant"`. The mismatch left the batched flatten path with a `PackedTensor` for normal assistant turns and a missing entry for reasoning/tool-call turns, crashing `PackedTensor.flattened_concat` on async multimodal GRPO runs. Gate the flush on `generation_token_ids` directly so the per-turn image list stays aligned with the postprocess loop's `turn_idx`, and add regression tests for the reasoning-only and function_call cases. Also drop the unused `processor` kwarg from the async GRPO call site. Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit 4c2537b) Signed-off-by: rohitrango <rohit.rango@gmail.com>
Add Args/Returns sections to image_to_data_url and encode_images_in_examples in nemo_rl/data/multimodal_utils.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit e9b89bc) Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com> (cherry picked from commit d60004f) Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
Ignore text-only function call outputs during image extraction and seed the first trainable turn with images from the initial input messages. Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: rohitrango <rohit.rango@gmail.com>
Non-colocated 2n8g layout (vLLM TP=8 on node 1, Megatron TP=2/EP=8/CP=2
on node 2) for the single-turn Circle-Click NeMo-Gym environment.
Model points at the HF repo (nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-BF16);
train/eval data_path use /path/to/{train,eval}_dataset.jsonl placeholders
so users wire in their own manifests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
The multimodal postprocessing path in _attach_multimodal_data_to_user_message assumes a placeholder-style processor: it reconstructs imgs_sizes / num_frames and builds the pixel_values PackedTensor with pad_to_max_shape=True. A non-placeholder VLM (e.g. Qwen2-VL / LLaVA-style) would silently produce wrong multimodal tensors instead of erroring. Fail loud at NemoGym.__init__ so the misconfiguration is caught at actor construction, well before any rollout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: rohitrango <rohit.rango@gmail.com>
…click recipe Adds the required defaults: ../../vlm_grpo_3B_megatron.yaml key so the recipe passes the configs-minimize-check pre-commit hook (all recipes under examples/configs/recipes/**/*.yaml must inherit from an exemplar). Drops the standalone header preamble; provenance now lives in the adjacent clevr/mmpr sibling recipes and the commit history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: rohitrango <rohit.rango@gmail.com>
Signed-off-by: rohitrango <rohit.rango@gmail.com>
The circle-click recipe YAML landed without the driver script and suite entry that tests/unit/test_recipes_and_test_suites.py requires, so test_all_recipe_yamls_accounted_for_in_test_suites failed on a 229 vs 228 count mismatch. Add the missing driver. circle_click is a NeMo-Gym env, so it runs through run_grpo_nemo_gym.py rather than run_vlm_grpo.py, and the script regenerates its data via the resources server's generate_data.py (the committed example.jsonl has 5 rows and no agent_ref) with disjoint train/eval seeds. List it in disabled.txt rather than nightly.txt for now: the recipe has not been run end to end, so its reward threshold is an unvalidated smoke bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
(cherry picked from commit c3b8dacb3320214e8d5404fb4f31de01a63df2f8) Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com>
Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com>
Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com>
|
Summary for the review above (the review body was lost on submit; GitHub does not allow adding one afterwards). Reviewed with a team of agents (core data structures, GRPO/rollouts/async integration, config and guideline conformance, tests, an independent bug scan) plus adversarial verification passes that re-derived each finding from source before inclusion. Scope. ~4,336 added lines (production +1,977 / tests +2,359). Roughly 1,030 of those belong to the payload-metrics subsystem: a 403-line module, a 327-line test file, and ~40 plumbing sites across 9 production files (115 references total). Its dependency direction is one-way — nothing in Correctness. One issue, at bf16 pre-cast. Applied at 3 of 9 structurally identical Unconditional changes. The Documentation. Investigated and excluded after verification, listed so they are not re-raised: Generated by Claude Code |
Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com>
Four follow-ups to the dedup review, none of which change production behavior for any shipped configuration. - test_grpo: add the required policy.precision to mock_grpo_components. bd4a2a2 replaced a hardcoded torch.bfloat16 with _policy_dtype(), which indexes policy_config["precision"] eagerly on every GRPO step, so the shared fixture failed 25 pre-existing tests with KeyError: 'precision'. - PackedTensor.to_dtype: skip non-floating-point segments. The cast allowlist was widened to pixel_values_videos without the float guard that was its stated precondition, leaving a string allowlist as the only thing keeping integer media metadata (grid sizes, frame counts) out of a float cast. - GenerationConfig: rename debug_payload_metrics to _debug_payload_metrics, matching the _pad_token_id / _mtp_weights_from_refit convention for keys populated internally rather than by the user. grpo.setup() overwrites the value unconditionally, so a user-set key was silently discarded. Readers now use bool(cfg.get(...)) with no invented default, matching vllm_worker.py's read of _mtp_weights_from_refit. - test_rollouts: the two multi-turn tests parametrized deduplicate_multimodal_data but asserted nothing that differed between legs. Capture the forwarded flag and the presence of the policy-ready media payload so both legs prove distinct behavior. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
yfw
left a comment
There was a problem hiding this comment.
Follow-up on bd4a2a29: verified the responses to both reviews against the code. Nearly everything checks out — the deletions are clean (task-index recovery is equivalent across six resume scenarios, save_replay_buffer removal is behavior-identical for every in-tree config, and the flattened_concat guard turns the old silent row misalignment into a loud failure). We've resolved the threads that landed.
One new issue below: the bf16 → policy.precision fix, which was the right call, leaves test_grpo.py with 25 failing tests. Flagging it because CI hasn't run yet, so there's currently no signal.
Generated by Claude Code
…r attach helpers Follow-ups to the two open documentation/coverage threads. - PackedTensor: add a worked CSR example. The numbers are derived from a real G=3 expansion rather than transcribed, so six logical rows over nine segment references map onto three physical tensors. Describes the mechanism that actually ships (deep-copied rows plus _prepare_multimodal_sharing aliasing the media leaves, re-interned by concat), since PackedTensor.repeat_interleave was removed. - PackedTensor.to: record why a device move retains provenance while to_dtype mints fresh provenance -- concat re-interns on provenance alone, so a value-changing operation must not keep it. Notes that the mixed-device corollary is currently unreachable and would fail loudly. - test_rollouts: cover the two untested paths in attach_initial_nemo_gym_image_payloads and attach_image_model_inputs_to_message -- the "no user message" ValueError, that rollout token_ids survive while media is packed, and the no-op guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
768b39f to
6bb0290
Compare
yfw
left a comment
There was a problem hiding this comment.
approved up until my last 2 commits
|
/ok to test 90c467e |
CI runs `pre-commit run --all-files`, which includes `ruff check --select I` (isort) and `ruff-format` at the pinned v0.9.9. Those are separate hooks from the `ruff check` governed by pyproject's `select = ["D", "F"]`, so this drift was not caught by a plain `ruff check`. Import ordering: multimodal_payload_metrics.py, test_rollouts.py, test_config_validation.py. Formatting: grpo.py, test_llm_message_utils.py. Mechanical only -- no logic changes. The one remaining format diff is inside the Automodel submodule, which is not tracked by this repo and so is out of scope for pre-commit here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test bd2a314 |
test_config_v2_same_as_v1 compares each v1 reference config against its
Pydantic v2 model_dump and fails when the model introduces defaults the
reference lacks. Adding deduplicate_multimodal_data and debug_payload_metrics
to the GRPOConfig BaseModel therefore broke it:
assert 2 == 0
+ where 2 = len(['grpo.deduplicate_multimodal_data',
'grpo.debug_payload_metrics'])
Per the config-conventions skill, a change to an exemplar covered by
test_config_v2.py must also update the matching reference config. The trigger
is adding fields to the BaseModel, not editing the exemplar YAML itself, so
this applies even though the PR left examples/configs/grpo_math_1B.yaml
untouched. Values and comments match the exemplar entries in vlm_grpo_3B.yaml.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test a23a8d1 |
Deduplicate multimodal GRPO payloads
What this changes
compact physical tensor store, while retaining normal logical batch semantics.
dynamic batching, sequence packing, DP sharding, async replay, DAPO assembly,
and replay-buffer checkpoint restore.
transfers retain savings even when packing or sharding splits prompt groups.
qualification is currently Nemotron image and Qwen2.5-VL image; audio/video
behavior is covered by focused data-structure tests rather than model runs.
Gym request payloads are not expected to shrink because Gym receives each
request independently; Gym return, replay, and policy-training boundaries do.
stale turn-0
vllm_content; later turns use the current conversation tokens andmedia. This behavior correction is unconditional, not gated by deduplication.
pixel_valuesfrom fp32 to the configured policyprecision before Ray transfer (bf16 in the shipped recipes), matching Omni MR
!12 and halving physical pixel-tensor bytes for bf16. Preprocessing, Gym return,
and replay storage retain fp32; both dedup modes use the same cast.
deduplication; payload metrics remain debug-only and disabled in recipes.
Representation and safety contract
PackedTensorstores physical segments plus CSR-like row offsets and segmentindices. Each physical segment also carries opaque provenance.
Provenance identity is the sole evidence used to re-intern physical segments;
tensor values are not compared. Value-preserving copies and device moves retain
provenance, while value-changing dtype conversion issues fresh provenance. Missing media rows are explicit,
and correlated media keys preserve per-row logical counts and ordering.
Flag-off retains the legacy expanded logical representation. Both modes use the
same configured
pixel_valuesdtype for policy-bound transfers, so dedup A/Bruns do not differ in dtype. Legacy pickles and checkpoints remain readable.
Compact replay checkpoints also remain readable with the flag off because the
compact representation is self-describing inside each saved message log.
Quantified G=16 evidence
The W&B campaigns below quantify deduplication before the bf16 pre-cast was
ported. They remain the structural off/on evidence. The pre-cast applies equally
to both modes and independently halves physical policy-bound
pixel_valuesbytes; the focused protocol-5 test above validates the current combined path.
Nemotron Omni 30B, Megatron, async NeMo Gym
4 nodes, 8 GPUs/node, TP=2, EP=16, CP=2, 10 steps. Dedup off/on jobs
14856877/14856876completed with exact logical trajectory, reward, and tokenparity. TMPE mean/max was
1.022114/1.028493off and1.021894/1.027261on.W&B: off,
on.
An independently repeated one-step final-telemetry pair (
14869430/14869431)matched all 16 logical rows, rewards, token counts, masks, advantages, content,
and agent references. TMPE was
1.014925off and1.014564on. Its reductionswere 620.02x at Gym return, 15.78x at replay push/sample, 13.41x rollout-to-policy,
and 3.77-3.85x across policy shards.
Qwen2.5-VL 3B, Megatron, native rollout
1 node, TP=1, DP=8, G=16, 10 steps. Jobs
14858320/14858321matched logicaltrajectories, rewards, and generated tokens exactly. Mean loss was
0.0748708278off and0.0748722717on; maximum paired loss delta was1.21668e-5. TMPE mean/max was1.022007/1.025749off and1.021798/1.024828on. Serialized reductions were 14.97x at prompt repeat,17.05x at vLLM generation, 15.06x rollout-to-policy, and approximately 1.99x
across policy shards.
W&B: off,
on.
Qwen2.5-VL 3B, AutoModel, native rollout
1 node, DP=8, G=16, 10 sampled steps. Jobs
14979492/14979493completed allsteps. The first complete generation-to-training step matched exactly; later
independently updated sampled trajectories diverged, so this qualifies the
AutoModel data path and multi-step health, not strict multi-step A/B identity.
Serialized reductions were 14.96x prompt repeat, 17.05x vLLM generation,
15.29x rollout-to-policy, and approximately 1.99x at policy boundaries.
W&B: off,
on.
Gemma 3 4B, AutoModel, transport-only diagnostic
A 10-step native-rollout G=16 pair on pre-rebase candidate
5496708ealsoexercised the generic AutoModel transport path. The first-step trajectory hash
matched exactly. Averaged across ten steps, serialized payloads fell 43.12x at
the exact sharded vLLM-generation Ray boundary, 15.68x in the rollout-to-policy
batch, and approximately 2.00x at the sharded policy logprob, reference-logprob,
and training Ray boundaries. The 2x shard-local result is expected because 16
logical generations were distributed over eight AutoModel DP workers.
This is transport-only evidence. Gemma is not included in model correctness or
performance qualification because its unrelated vLLM/AutoModel token-logprob
mismatch was catastrophic with deduplication both disabled and enabled.
W&B: off,
on.
Current-#3414 validation
956 passed, 6 skipped(SLURM
15214209). This included full GRPO, PPO, distillation, Gym/replay,batching, AutoModel data, vLLM utils, config validation, serialization, and
payload-metric suites.
2 passed(SLURM15214753), including model-ingress,logprob/loss/gradient parity and distributed-checkpoint round-trip.
c3b8dacbonto the latestfeat: Multimodal nemo gym compatible grpo pipeline #3414 head
f27dc6d6;git range-diffconfirms the product patch is identical.The intervening feat: Multimodal nemo gym compatible grpo pipeline #3414 delta only changes PPO documentation/configuration,
ppo.py, and dataset utilities, with no file overlap with this patch.91959244, Ruff format/check passed and Pyreflyreported zero shown errors across 219 modules (255 pre-existing ignored
diagnostics) in SLURM
15222427. The focusedGRPO/PPO/data/Gym/replay/backend/config/payload suite passed on GPU with
1004 passed, 6 skippedin SLURM15222850.d95bb1aeports Omni MR !12 to GRPO withoutmaterializing deduplicated rows. Focused validation: 196 generic GRPO/data/
payload tests passed (SLURM
15234623), 39 AutoModel data tests passed(
15234855), and 41 Megatron data tests passed with one expected 2-GPU skip(
15234856). Ruff format/check passed; Pyrefly reported zero errors on thechanged whitelisted module. The payload test verifies exactly 2x fewer
physical
pixel_valuesbytes plus lower protocol-5 serialized size.29de7b52removes the standalone design document andredundant per-recipe debug defaults. The resulting complete configuration set
passed with
547 passed, 6 skippedin SLURM15224236.c3b8dacbpatch foundno code-level merge blockers.
15216656/15217828completed two steps each. The first trajectory hashmatched exactly; later independently sampled trajectories diverged after
optimizer/refit, as expected for this non-frozen smoke. Aggregate serialized
reductions were 14.99x prompt repeat, 17.06x vLLM generation, 15.30x
rollout-to-policy, 1.994x policy/reference logprobs, and 1.991x policy train.
W&B: off,
on.
EP=16, CP=2 job
15219896completed two steps per side on four 8-GPU nodes. Alllosses were finite and TMPE stayed healthy: 1.01897-1.01950 off and
1.02197-1.02418 on. Aggregate serialized reductions were 406.68x at Gym
return, 15.67x replay push, 15.69x replay sample, 15.28x rollout-to-policy,
3.97x policy logprobs, and 3.96x policy train; Gym request was exactly 1.00x.
These were independent sampled runs, so the frozen 10-step campaign above,
rather than this smoke, supplies the strict parity claim. W&B:
off,
on.
Evaluation limits
vLLM/AutoModel token-logprob mismatch is catastrophic with dedup both off and
on. Its transport-only diagnostic is reported separately above.
coverage and are intentionally media-type agnostic.
change. Megatron CP=2 is qualified; CP>2 is not claimed.
nontrivial work. It is intended for qualification/debugging, not production.
Test plan
empty rows, missing keys, cross-key alignment, and differing equivalence groups.
normalization and shard-local re-interning.
replay/DAPO/checkpoint restore, and flag-off behavior.