fix(vlm): propagate NeMo Gym dynamic image padding - #3738
Closed
youngeunkwon0405 wants to merge 3 commits into
Closed
fix(vlm): propagate NeMo Gym dynamic image padding#3738youngeunkwon0405 wants to merge 3 commits into
youngeunkwon0405 wants to merge 3 commits into
Conversation
Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
youngeunkwon0405
left a comment
Contributor
Author
There was a problem hiding this comment.
This PR was reviewed by a coordinated team of 4 specialized agents (RL-codebase expert, test reviewer, independent bug scanner, and a devil's-advocate verification pass). Summary:
- The fix is correct and needed. Pre-PR, the dedup attachment path always invoked the processor with stacking semantics, breaking heterogeneous multi-image gym prompts even when the environment requested padding; the gym-actor side already honored the flag. All three production call sites are updated and no others exist.
- No bugs found. Config key path,
master_config.envtyping at all three call sites, and the downstreamattach_image_model_inputs_to_messagekeyword signature were all verified against the code. Ruff check/format pass. The updated unit test was run locally and passes. - The 5 inline comments are all low-severity polish (config-read convention + shared helper, two optional verified test hardenings, a docstring nit) plus one pre-existing out-of-scope observation. Nothing blocks merge.
Validation evidence (W&B sync/async runs + Slurm job) is appropriate for a correctness fix — thanks for including it.
Generated by Claude Code
5 tasks
Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
Contributor
Author
|
/ok to test 36ed044 |
youngeunkwon0405
marked this pull request as ready for review
August 21, 2026 02:44
Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
Contributor
Author
|
/ok to test ac228c1 |
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.
What does this PR do?
Propagates the existing NeMo-Gym
pad_dynamic_image_shapesoption through the initial deduplicated multimodal attachment path.When
grpo.deduplicate_multimodal_data=true, GRPO attaches the initial image-model inputs before repeating prompts. That path previously calledattach_image_model_inputs_to_messagewithout forwarding the environment's dynamic-padding option. A heterogeneous multi-image example could therefore attempt to stack different image resolutions before rollout even when the NeMo-Gym environment requested padding.Before this fix, propagation stopped at the algorithm-to-rollout-helper boundary:
The synchronous training, validation, and asynchronous collection call sites invoked
attach_initial_nemo_gym_image_payloadswithout the flag. That helper then invokedattach_image_model_inputs_to_messagewithout the flag as well, so the target function silently used its existingFalsedefault. This early deduplicated attachment shortcut is separate from the normal NeMo-Gym environment preprocessing path, which already carried the option correctly.After this fix, the complete chain is:
This change:
pad_dynamic_image_shapesargument toattach_initial_nemo_gym_image_payloads;env.nemo_gym.pad_dynamic_image_shapesfrom synchronous training, validation, and asynchronous trajectory collection; andFalsedefault for every caller that does not opt in.This PR is independent of the 32-GPU recipe PR and independent of #3721.
Issues
None.
Validation
uvx --from ruff==0.9.9 ruff check nemo_rl/algorithms/async_utils/trajectory_collector.py nemo_rl/algorithms/grpo.py nemo_rl/experience/rollouts.py tests/unit/experience/test_rollouts.pyuvx --from ruff==0.9.9 ruff format --check nemo_rl/algorithms/async_utils/trajectory_collector.py nemo_rl/algorithms/grpo.py nemo_rl/experience/rollouts.py tests/unit/experience/test_rollouts.pyPYTHONPATH=. uv run --extra nemo_gym --group test pytest -q --noconftest tests/unit/experience/test_rollouts.py -k attach_initial_nemo_gym_image_payloads— 3 passed6370288—COMPLETED 0:0Before your PR is "Ready for review"