Skip to content

fix(vlm): dispatch async GRPO from VLM entrypoint - #3740

Merged
terrykong merged 1 commit into
mainfrom
codex/vlm-async-grpo-dispatch
Aug 21, 2026
Merged

fix(vlm): dispatch async GRPO from VLM entrypoint#3740
terrykong merged 1 commit into
mainfrom
codex/vlm-async-grpo-dispatch

Conversation

@youngeunkwon0405

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the VLM GRPO entrypoint so grpo.async_grpo.enabled=true selects async_grpo_train instead of silently continuing through the synchronous trainer.

Previously, examples/run_vlm_grpo.py unconditionally called grpo_train. An async VLM configuration could therefore reserve a non-colocated rollout pool while executing synchronous policy updates, and the run would still be labeled async in performance metrics because the trainer received the enabled config flag.

The VLM entrypoint now mirrors the established async dispatch in examples/run_grpo.py and examples/nemo_gym/run_grpo_nemo_gym.py:

  • reject dynamic sampling, reward scaling, reward shaping, and multiple dataloaders when async GRPO is enabled;
  • call async_grpo_train with trajectory-age, teacher-worker, and alias mappings from setup;
  • forward the multimodal processor to both trainers; and
  • preserve the existing synchronous path when async GRPO is disabled.

This is a framework-only follow-up to review feedback on #3737. It contains no recipe changes.

Issues

None.

Validation

  • ruff check examples/run_vlm_grpo.py tests/unit/test_config_validation.py
  • ruff format --check examples/run_vlm_grpo.py tests/unit/test_config_validation.py
  • pytest -q --noconftest tests/unit/test_config_validation.py -k 'multimodal_launchers_forward_processor_to_both_trainers or vlm_launcher_dispatches_on_async_grpo_enabled'

The focused Linux container run completed successfully (3 passed; Slurm job 6375778, COMPLETED 0:0). The tests verify that the async-enabled branch calls async_grpo_train, the fallback branch calls grpo_train, and both VLM trainers receive processor=processor.

Before your PR is "Ready for review"

Pre checks:

  • Read and followed the contributor guidelines
  • Added focused regression coverage
  • Ran repository-pinned lint and formatting checks
  • No documentation update is needed because the public CLI and configuration schema are unchanged

Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

/ok to test 7f00f9e

@youngeunkwon0405 youngeunkwon0405 added the CI:L1 Run doctests, unit tests, and functional tests label Aug 20, 2026
@youngeunkwon0405 youngeunkwon0405 self-assigned this Aug 20, 2026

@youngeunkwon0405 youngeunkwon0405 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — reviewed by a coordinated agent team (rl-expert, test-agent, bug-finder, devil's advocate); zero actionable findings.

Verified:

  • The async dispatch block mirrors run_grpo.py and run_grpo_nemo_gym.py exactly (same 4 guards, same messages, same kwargs).
  • All 16 kwargs check out against async_grpo_train; the setup() 13-tuple unpack rename is correct.
  • Every shipped VLM config chain roots at a yaml that defines data.use_multiple_dataloader and the async_grpo block, so the new guard reads are safe.
  • processor is genuinely consumed in the async path (trajectory collector + validation) with full parity to the sync trainer — no silent VLM degradation.
  • Both tests fail on main and pass on this branch, so they pin the fix. Lint (ruff check + ruff format --check) passes.

Nice preventive fix — before this change an async-enabled VLM config silently ran synchronous training.

Generated by Claude Code

@youngeunkwon0405
youngeunkwon0405 marked this pull request as ready for review August 20, 2026 23:35
@youngeunkwon0405
youngeunkwon0405 requested review from a team as code owners August 20, 2026 23:35
@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

/ok to test 7f00f9e

@youngeunkwon0405 youngeunkwon0405 added CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) and removed CI:L1 Run doctests, unit tests, and functional tests labels Aug 20, 2026
@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

/ok to test 7f00f9e

@rohitrango

Copy link
Copy Markdown
Contributor

yikes! how did we miss this?

lgtm!

@youngeunkwon0405
youngeunkwon0405 requested review from kajalj22 and removed request for kajalj22 August 21, 2026 02:41
@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

Hi @terrykong, can we merge this fix, please? It is a very simple, straightforward fix.

@terrykong
terrykong enabled auto-merge (squash) August 21, 2026 03:45
@terrykong
terrykong merged commit 14ce486 into main Aug 21, 2026
132 of 134 checks passed
@terrykong
terrykong deleted the codex/vlm-async-grpo-dispatch branch August 21, 2026 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants