-
Notifications
You must be signed in to change notification settings - Fork 542
feat(vlm): route VLM GRPO through TQ trainer when data_plane.enabled #2957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e2821ad
4a50fb3
002bbae
12b0abc
3de769e
6a153bf
1fe782d
4603365
a18956d
60c19aa
0e2ea50
79da085
8d5e0fd
530eca4
50990af
5a41608
4bcb483
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| defaults: vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1.yaml | ||
| data_plane: | ||
| enabled: true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| defaults: vlm_grpo-qwen3.5-35ba3b-geo3k-2n8g-automodel-ep16.yaml | ||
| data_plane: | ||
| enabled: true |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,23 @@ | |||||||||||||||||||
| from nemo_rl.utils.timer import Timer | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def _select_trainer(master_config: MasterConfig): | ||||||||||||||||||||
| """Pick the synchronous trainer based on ``data_plane.enabled``. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Mirrors ``run_grpo.py`` so the VLM launcher routes through the same | ||||||||||||||||||||
| TransferQueue-backed sibling trainer (``grpo_train_sync``) when the | ||||||||||||||||||||
| data plane is enabled, and otherwise uses the legacy ``grpo_train``. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| dp_cfg = master_config.data_plane or {} | ||||||||||||||||||||
| if dp_cfg.get("enabled", False): | ||||||||||||||||||||
| from nemo_rl.algorithms.grpo_sync import grpo_train_sync | ||||||||||||||||||||
|
|
||||||||||||||||||||
| print("🚀 Running synchronous VLM GRPO training (TransferQueue)") | ||||||||||||||||||||
| return grpo_train_sync | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we have a plan to update the file name? the current naming is quite confusing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeap, I'd expect this issue would be fixed once legacy path is retired. |
||||||||||||||||||||
| print("🚀 Running synchronous VLM GRPO training (legacy)") | ||||||||||||||||||||
| return grpo_train | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def parse_args() -> tuple[argparse.Namespace, list[str]]: | ||||||||||||||||||||
| """Parse command line arguments.""" | ||||||||||||||||||||
| parser = argparse.ArgumentParser(description="Run GRPO training with configuration") | ||||||||||||||||||||
|
|
@@ -110,6 +127,20 @@ def main() -> None: | |||||||||||||||||||
| processor, config.data, config.env, is_vlm=True | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Pick the policy factory at the launcher level so the legacy trainer | ||||||||||||||||||||
| # stays data-plane-agnostic (architectural invariant — see | ||||||||||||||||||||
| # tests/unit/data_plane/test_architecture_invariants.py). | ||||||||||||||||||||
| _dp_cfg = config.data_plane or {} | ||||||||||||||||||||
| if _dp_cfg.get("enabled", False): | ||||||||||||||||||||
| from nemo_rl.models.policy.tq_policy import TQPolicy | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def _make_policy(**kwargs): | ||||||||||||||||||||
| return TQPolicy(**kwargs, dp_cfg=_dp_cfg) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| _policy_factory = _make_policy | ||||||||||||||||||||
| else: | ||||||||||||||||||||
| _policy_factory = None # setup() defaults to plain Policy | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with rl_init_timer.time("setup"): | ||||||||||||||||||||
| ( | ||||||||||||||||||||
| policy, | ||||||||||||||||||||
|
|
@@ -125,7 +156,14 @@ def main() -> None: | |||||||||||||||||||
| master_config, | ||||||||||||||||||||
| teacher_worker_groups, | ||||||||||||||||||||
| alias_to_group_alias, | ||||||||||||||||||||
| ) = setup(config, tokenizer, dataset, val_dataset, processor=processor) | ||||||||||||||||||||
| ) = setup( | ||||||||||||||||||||
| config, | ||||||||||||||||||||
| tokenizer, | ||||||||||||||||||||
| dataset, | ||||||||||||||||||||
| val_dataset, | ||||||||||||||||||||
| processor=processor, | ||||||||||||||||||||
| policy_factory=_policy_factory, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| rl_init_timer.record("total", time.perf_counter() - main_start) | ||||||||||||||||||||
| rl_init_metrics = rl_init_timer.get_timing_metrics(reduction_op="sum") | ||||||||||||||||||||
|
|
@@ -170,8 +208,9 @@ def main() -> None: | |||||||||||||||||||
| processor=processor, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| else: | ||||||||||||||||||||
| print("🚀 Running synchronous GRPO training") | ||||||||||||||||||||
| grpo_train( | ||||||||||||||||||||
| # ``_select_trainer`` prints which sync trainer it picked. | ||||||||||||||||||||
| trainer = _select_trainer(master_config) | ||||||||||||||||||||
| trainer( | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The sibling launcher wraps this call in # run_grpo.py:241-245
# grpo_train_sync defers checkpoint finalization to the checkpointer's
# background threads; the context manager guarantees they are flushed on
# exit. (grpo_train also flushes internally; shutdown() is idempotent.)
with checkpointer:
trainer(( Lines 241 to 245 in 4bcb483
The omission was harmless before this PR, because this launcher only ever called
Suggested change
(the call's arguments need one more level of indentation) |
||||||||||||||||||||
| policy, | ||||||||||||||||||||
| policy_generation, | ||||||||||||||||||||
| dataloader, | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -462,10 +462,22 @@ def _validate_multimodal_dedup_capability(master_config: MasterConfig) -> None: | |
| "grpo.deduplicate_multimodal_data=true is currently qualified " | ||
| "only with policy.generation.backend=vllm." | ||
| ) | ||
| if (master_config.data_plane or {}).get("enabled", False): | ||
| # The data plane carries deduplicated payloads: ``PackedTensor.to_wire`` | ||
| # emits one wire row per *logical* row and walks segments under dedup, so | ||
| # the wire format itself is not the constraint. The one gap is NeMo-Gym: | ||
| # ``grpo_train_sync`` does not call | ||
| # ``attach_initial_nemo_gym_image_payloads``, which supplies the initial | ||
| # image tensors a Gym dataset omits from ``extra_env_info``. That helper is | ||
| # itself gated on ``should_use_nemo_gym``, so non-Gym recipes never needed | ||
| # it and are unaffected. | ||
| if (master_config.data_plane or {}).get("enabled", False) and ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The premise of this relaxation does not hold: on the TQ trainer, deduplication never happens at all, so the flag becomes a silent no-op rather than a supported combination.
# grpo.py:3069-3073
batch.repeat_interleave(
master_config.grpo.num_generations_per_prompt,
share_immutable_media=(master_config.grpo.deduplicate_multimodal_data),
)
This is live in a recipe this PR adds: Worth knowing for whichever fix you pick: even with sharing enabled, Three options, in order of effort: pass
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rohit: Since your PR already added wiring for deduped I think the better raise condition for this is to check if @terrykong might need your opinion on this. |
||
| should_use_nemo_gym(master_config) | ||
| ): | ||
| raise NotImplementedError( | ||
| "grpo.deduplicate_multimodal_data=true is currently supported " | ||
| "only when data_plane.enabled=false." | ||
| "grpo.deduplicate_multimodal_data=true with data_plane.enabled=true " | ||
| "is not supported for NeMo-Gym runs: the TransferQueue trainer does " | ||
| "not attach the initial Gym image payloads. Non-Gym recipes are " | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Three user-facing copies of the old rule survive this relaxation and now contradict both the code and the recipe this PR adds (which sets
All three say "requires the vLLM generation backend and Separately, VLM GRPO over the data plane is a new supported configuration and nothing under |
||
| "supported." | ||
| ) | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this PR be validated on nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-BF16 ?
The implementation from #3290 explicitly forbids models like Nano-Omni from working with the DataPlane, see
nemo_rl/data_plane/worker_mixin.py:137. The check was added because PackedTensor objects could not be passed through the TQ, which is now implemented here.