[cfg, recipe, tests] feat: wire DAPO overlong reward buffer on V1 omni loop - #502
[cfg, recipe, tests] feat: wire DAPO overlong reward buffer on V1 omni loop#502OnePunchMonk wants to merge 10 commits into
Conversation
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new CPU test test_dapo_overlong_reward_on_cpu.py currently pulls gpt2 via AutoTokenizer.from_pretrained, which can introduce network/offline flakiness in CI and should be made self-contained.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Wires DAPO “overlong response” shaping configuration through verl_omni’s reward config so CLI overrides under reward.reward_kwargs.* reach verl’s registered DAPORewardManager, and adds smoke/CPU contract tests to ensure the overlong penalty path is actually exercised.
Changes:
- Add
reward.reward_kwargs(overlong buffer cfg +max_resp_len) toverl_omnireward config and propagate into generated trainer configs. - Add/adjust CPU tests to validate (a) algo config fields survive Hydra/dataclass conversion and (b) overlong shaping changes reward under
DAPORewardManager. - Add DAPO tiny-random V1 smoke script and DAPO example docs/launcher contract assertions.
File summaries
| File | Description |
|---|---|
| verl_omni/utils/config.py | Adds a warning guardrail when reward_manager=dapo is paired with GSPO loss mode. |
| verl_omni/trainer/config/reward/reward.yaml | Declares reward.reward_kwargs.* so reward-manager kwargs are not silently dropped. |
| verl_omni/trainer/config/algorithm.py | Updates OmniAlgoConfig to inherit upstream AlgoConfig for online/DAPO fields. |
| verl_omni/trainer/config/_generated_omni_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| verl_omni/trainer/config/_generated_omni_megatron_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| verl_omni/trainer/config/_generated_diffusion_veomni_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| verl_omni/trainer/config/_generated_diffusion_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| tests/workers/config/test_omni_config_on_cpu.py | Extends CPU config tests to assert inherited AlgoConfig fields survive Hydra conversion. |
| tests/utils/test_qwen3_omni_dapo_launcher_on_cpu.py | Adds contract tests for the AVQA launcher and the tiny-random DAPO smoke script, including overlong kwargs on the smoke. |
| tests/utils/test_dapo_overlong_reward_on_cpu.py | Adds a direct-unit CPU test ensuring overlong shaping changes the DAPO reward output. |
| tests/utils/test_config_on_cpu.py | Adds tests for the new GSPO+DAPO warning and the vanilla+DAPO non-warning case. |
| tests/special_e2e/run_dapo_qwen3_omni_thinker_lora_v1_smoke.sh | Adds a 2-GPU tiny-random DAPO V1 smoke that enables overlong shaping via reward.reward_kwargs.*. |
| examples/dapo_trainer/README.md | Documents the Thinker DAPO recipe and clarifies Phase 2 overlong shaping behavior and where it applies. |
| examples/dapo_trainer/qwen3_omni/run_qwen3_omni_thinker_dapo_lora_v1.sh | Adds the AVQA Thinker DAPO launcher (naive manager; dynamic sampling disabled). |
| docs/index.md | Adds the DAPO example page to the docs toctree. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| tokenizer = AutoTokenizer.from_pretrained("gpt2") | ||
| return DAPORewardManager(config, tokenizer, _compute_score) |
|
Marking this reviewable once #456 (Phase 1) lands. This branch is stacked on Everything else in this PR is otherwise ready: CPU tests pass, GPU e2e smoke passes (see the PR description for the flashinfer environment detour — filed as #505/#506, unrelated to this PR's code), and the config-plumbing/reward-manager wiring is verified against upstream |
Assisted-by: AI tooling Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Assisted-by: AI tooling Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
…i loop (verl-project#446 Phase 2) Phase 1 (verl-project#456) left the overlong reward buffer disabled everywhere; verl's registered DAPORewardManager already reads reward.reward_kwargs.overlong_buffer_cfg and reward.reward_kwargs.max_resp_len off the config, but verl_omni's reward.yaml never declared reward_kwargs, so any override there was silently dropped for the autogenerated trainer configs. - Add reward.reward_kwargs (overlong_buffer_cfg + max_resp_len) to reward.yaml and regenerate the four _generated_*.yaml trainer configs. - Enable overlong shaping in the DAPO tiny-random smoke test (reward.reward_manager.name=dapo path only; the naive-manager AVQA example is unaffected and documented as such). - Add a CPU test that drives DAPORewardManager directly and asserts the overlong penalty changes the reward on a truncated dummy response — the Phase 2 exit criterion from verl-project#446. - Split the shared launcher-contract test helper so the "no overlong" check only applies to the naive-manager example, and add overlong-specific assertions for the dapo-manager smoke test. This does not touch dynamic sampling (Phase 3) or the reward-loop metrics plumbing beyond what DAPORewardManager already logs in reward_extra_info. Draft: opened for early feedback while GPU LoRA validation on real Qwen3-Omni-30B-A3B-Instruct + gsm8k is still pending (see PR description). Co-authored-by: Claude Code
Co-authored-by: Claude Code
7177767 to
7dc3bd9
Compare
GPU smoke re-run after rebase onto #456's latest headRe-ran the Phase 2 GPU e2e smoke on Modal (2×A10G, CUDA 12.4.1 base + the same NUM_GPUS=2 TOTAL_TRAIN_STEPS=2 bash tests/special_e2e/run_dapo_qwen3_omni_thinker_lora_v1_smoke.shPer-sample DAPO reward scores again varied across the batch ( CPU tests also still pass on this head: python3 -m pytest tests/utils/test_dapo_overlong_reward_on_cpu.py tests/utils/test_qwen3_omni_dapo_launcher_on_cpu.py -q
# 3 passedMarking this ready for review now that it's rebased onto #456's latest and |
Signed-off-by: Avaya Aggarwal <119044997+OnePunchMonk@users.noreply.github.com>
Summary
Implements Phase 2 of #446 (Qwen3-Omni Thinker DAPO): wires the overlong
response-length penalty into the V1 omni reward path.
Stacked on #456 (Phase 1) — not yet merged. This branch is rebased onto
wenzhe/qwen3-omni-thinker-dapoatbc0bee6(current #456 head), so the diffagainst
mainincludes #456's commits. Please review only the two commits ontop (
feat: wire DAPO overlong reward buffer on V1 omni loop (#446 Phase 2),chore: trim redundant comments) until #456 lands; I'll rebase and shrink thediff once it merges.
2026-09-04 update: rebased onto #456's latest head (it had moved since
this PR was opened — the Phase-1 GPU smoke script was removed there per
review feedback, since the existing GSPO smoke covers the same pipeline).
Phase 2 restores that smoke script under its own scope, since it's this PR's
own e2e coverage for the overlong buffer, not a reintroduction of the removed
Phase-1 smoke. CPU tests pass post-rebase; the GPU e2e run below predates the
rebase and should be re-run before merge — I don't have further Modal/GPU
access in this pass.
What's actually wired
verl's registereddaporeward manager(
verl.experimental.reward_loop.reward_manager.dapo.DAPORewardManager) alreadyreads
config.reward.reward_kwargs.overlong_buffer_cfgandconfig.reward.reward_kwargs.max_resp_len— butverl_omni'sreward.yamlnever declared a
reward_kwargskey, so any++reward.reward_kwargs.overlong_buffer_cfg.enable=trueoverride on the CLIwas silently swallowed rather than reaching the manager. This PR:
reward.reward_kwargs(overlong_buffer_cfg.{enable,len,penalty_factor,log},max_resp_len) toverl_omni/trainer/config/reward/reward.yaml, disabled bydefault, and regenerates the four
_generated_*.yamltrainer configs viascripts/generate_trainer_config.sh.(
tests/special_e2e/run_dapo_qwen3_omni_thinker_lora_v1_smoke.sh), sizedagainst its own
data.max_response_length=512. The naive-manager AVQAexample (
examples/dapo_trainer/qwen3_omni/run_qwen3_omni_thinker_dapo_lora_v1.sh)is untouched and documented as such — overlong shaping is a no-op under
reward.reward_manager.name=naive.tests/utils/test_dapo_overlong_reward_on_cpu.py, which drivesDAPORewardManagerdirectly (no Hydra/trainer bring-up) and asserts theoverlong penalty actually changes the reward on a truncated dummy response
— this is Phase 2's exit criterion from the issue.
tests/utils/test_qwen3_omni_dapo_launcher_on_cpu.pyso "no overlongconfig" only applies to the naive-manager example, and adds
overlong-specific assertions for the dapo-manager smoke test.
Not in scope: dynamic sampling (Phase 3), and any reward-loop metrics
plumbing beyond what
DAPORewardManageralready logs intoreward_extra_info(
overlong,overlong_reward).Test plan
CPU (all green locally):
GPU e2e (2-GPU tiny-random smoke, exercises the new
reward.reward_kwargs.*overrides end-to-end through the real V1 trainer loop):
Ran this on a Modal 2×A10G box against an improvised environment (CUDA
12.4.1 base + the exact pins from
.github/actions/gpu-smoke-prepare/action.yml),since I don't have access to CI's private
cu130registry image.First two attempts hit environment issues unrelated to this PR: a missing
apt package, then a genuine upstream bug in
flashinfer-python==0.6.16.post3(pulled in transitively via
vllm==0.27.0, no floor pinned anywhere in thisrepo) that crashes any
TP>=2GPU run at import time. Filed as #505, fixedin #506 (floors
flashinfer-python>=0.6.16.post4in thegpuextra) — seethose for the full root-cause writeup.
With
flashinfer-python==0.6.16.post4pinned (an environment-only override,nothing in this repo changed), the smoke test passes:
Per-sample DAPO reward scores varied across the batch (
-1.0,-0.3438,-0.0625), consistent with the overlong-buffer penalty grading responsesapproaching/at
max_resp_len=512(bufferlen=128, so penalty onset at~384 tokens, full -1.0 at 512).
One caveat I'm not glossing over: I could not find an explicit
step:N - reward/overlong...console-logger line in the captured output —grepped the full stdout+stderr for
step:and got zero matches for anymetric, not just overlong, so this looks like a console-logging/capture gap
in this V1 path rather than evidence the wiring is broken (the reward
computation itself clearly executed through the DAPO path, per the reward
scores above). Worth a maintainer's eyes on whether metrics are expected to
print to console here or only to the configured tracker backend.
AI usage disclosure
Claude Code was used to trace the config-plumbing gap, implement this change,
and write the tests described above. A human submitter (onepunchmonk) has
reviewed every changed line and will review the Modal GPU run before taking
this out of draft.