[recipe, tests, doc] feat: add BAGEL FlowGRPO V1 sync recipe - #511
[recipe, tests, doc] feat: add BAGEL FlowGRPO V1 sync recipe#511OnePunchMonk wants to merge 6 commits into
Conversation
…oject#389) Mechanical v0->v1 migration of the PickScore LoRA recipe, following the same pattern already used for SD3.5 (verl-project#296) and Wan2.2 (verl-project#466): swap main_diffusion -> main_diffusion_v1 and add trainer.use_v1=true trainer.v1.trainer_mode=sync. No adapter/loss changes. - examples/flowgrpo_trainer/bagel/run_bagel_pickscore_lora_v1.sh - tests/special_e2e/run_flowgrpo_bagel_pickscore_v1.sh (tiny-random GPU smoke, mirrors run_flowgrpo_bagel_pickscore.sh) - README: document the V1 recipe Co-authored-by: Claude Code
There was a problem hiding this comment.
🟢 Approval recommended
Changes are isolated to new recipe/test scripts and README documentation, and the new v1 e2e script cleanly mirrors the existing v0 BAGEL smoke test with the expected mechanical v1 trainer switch.
Pull request overview
Adds a BAGEL + FlowGRPO v1 trainer (sync) recipe and a matching tiny-random GPU smoke test, following the established “v0 → v1 mechanical migration” pattern used by existing diffusion v1 recipes (switch to main_diffusion_v1 and set trainer.use_v1=true, trainer.v1.trainer_mode=sync). Also documents the new v1 entrypoint in the BAGEL recipe README and links to the diffusion v1 guide.
Changes:
- Add a v1 sync BAGEL PickScore LoRA recipe script using
python -m verl_omni.trainer.main_diffusion_v1with v1 trainer flags. - Add a tiny-random GPU e2e smoke test script for the BAGEL v1 path, mirroring the existing BAGEL v0 e2e test but routed through the v1 entrypoint.
- Update the BAGEL recipe README to document how to run the new v1 sync recipe and reference the diffusion v1 guide.
File summaries
| File | Description |
|---|---|
| tests/special_e2e/run_flowgrpo_bagel_pickscore_v1.sh | Adds a v1 “tiny-random” GPU smoke test for BAGEL FlowGRPO PickScore using main_diffusion_v1 + trainer.use_v1=true + trainer.v1.trainer_mode=sync. |
| examples/flowgrpo_trainer/bagel/run_bagel_pickscore_lora_v1.sh | Adds the v1 sync counterpart of the BAGEL PickScore LoRA FlowGRPO recipe script (mechanical v0→v1 entrypoint + flags). |
| examples/flowgrpo_trainer/bagel/README.md | Documents the new v1 sync recipe and links to docs/start/diffusion_v1.md. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Claude Code
|
Ran the GPU smoke tests on Modal (4x A10G) to validate this against the v0 baseline. v0 control ( v1 sync ( at Also hit two environment-only issues along the way (not related to this PR, noting in case others hit them on Modal/similar sandboxes): a Ray placement-group deadlock caused by TransferQueue's default Will patch the |
… in BAGEL adapter _prompt_token_ids_to_batch looked up micro_batch["prompt_token_ids"], a key that is never written; every producer of this field (v0 and v1 alike) uses "prompts". This crashed the v1 sync path with a KeyError at the first rollout call. v0 never hit it because its data flow happened to populate prompt_token_ids elsewhere before reaching this adapter.
|
Pushed the Kicked off a fresh v0-vs-v1 side-by-side run on Modal to confirm the fix and get comparable step metrics. Both jobs got killed by a Modal client disconnect during Ray init, before either reached a training step, so I don't have new numbers to post yet. Not a code issue, just an infra hiccup on this run. Existing v0 baseline (from the earlier comment, still valid): Will rerun the v1 path and post updated metrics (plus a v0-vs-v1 plot) before this comes out of draft. |
Loss and reward curves from a joint 4x A10G run of the v0 and v1 BAGEL FlowGRPO PickScore smoke tests (8 steps each, tiny-random checkpoint), confirming the v1 sync migration reproduces v0 training behavior after the prompt_token_ids fix.
The plot doesn't belong as a tracked repo asset; posting it as a PR comment instead of committing it to docs/assets/.
|
cc: @zhtmike Do you think we need more step for verification |
|
@AndyZhou952 do you have the last bagel record? |
|
the reward should not be such low |
|
@knlnguyen1802 @zhtmike flagging for visibility: the v0-vs-v1 comparison table above is a tiny-random smoke test (randomly-initialized BAGEL checkpoint + dummy PickScore data, 8 steps), not a full-checkpoint training run. It confirms the v1 sync migration is mechanically correct and reproduces v0's trajectory, but the flat ~0.02 reward is expected for random weights on dummy data, not a signal about real PickScore reward quality. A full-checkpoint validation would need a separate run. |
Signed-off-by: Avaya Aggarwal <119044997+OnePunchMonk@users.noreply.github.com>
|
I will tackle the validation for this as requested by @knlnguyen1802 . And if you are interested, you may help to debug this as well: #561 |
|
Sure, will check #561 next! |
What does this PR do?
Claims the BAGEL + FlowGRPO (
sync) row from #389's "Need help" table:mechanical v0→v1 migration of the PickScore LoRA recipe, following the exact
pattern already used for SD3.5 (#296) and Wan2.2 (#466) — swap
main_diffusion→main_diffusion_v1and addtrainer.use_v1=true trainer.v1.trainer_mode=sync. No adapter/loss changes.Changes
examples/flowgrpo_trainer/bagel/run_bagel_pickscore_lora_v1.sh— v1 synccounterpart of
run_bagel_pickscore_lora.sh.tests/special_e2e/run_flowgrpo_bagel_pickscore_v1.sh— tiny-random GPUsmoke test, mirrors
run_flowgrpo_bagel_pickscore.sh(which validates thev0 path), swapped to the v1 entrypoint.
README: documents the new V1 recipe, cross-references
docs/start/diffusion_v1.md.Duplicate-work check
gh pr list --repo verl-project/verl-omni --state open --search "bagel"—no existing PR touches BAGEL's v1 path (hits are an unrelated BAGEL SFT PR
and an unrelated HunyuanImage-3.0 PR).
lists BAGEL FlowGRPO under "Need help".
Checklist Before Starting
gh pr list --repo verl-project/verl-omni --state open --search "bagel"[{modules}] {type}: {description}Test plan
GPU e2e (tiny-random smoke, validates the v1 entrypoint end-to-end):
v0 vs v1, same container, 4x A10G (both scripts run back to back in a
single Modal GPU allocation, 8 steps each, tiny-random BAGEL + dummy
PickScore data): both passed with matching training/reward trajectories.
Both runs returned exit code 0. Reward means track each other step for
step (same seeded data), confirming the v1 sync migration reproduces v0
training behavior. Loss curves differ slightly (v1's actor loss runs an
early-run pass before the first weight sync, per the v1 trainer's
warmup), same order of magnitude, same downward trend, no divergence.
This was found and fixed in
diffusers_training_adapter.py(see below andthe earlier PR comment) before this comparison could run at all — v1 was
crashing with a KeyError on every attempt until that fix landed.
AI usage disclosure
Claude Code was used to draft this migration (diffed against the already-
merged SD3.5 and Qwen-Image v1 recipes to confirm the exact mechanical
pattern) and the smoke test. A human submitter (onepunchmonk) has reviewed
every changed line and the Modal GPU run before taking this out
of draft.
Claude Code also found and fixed a real bug while testing the v1 path:
_prompt_token_ids_to_batchindiffusers_training_adapter.pyreadmicro_batch["prompt_token_ids"], a key nothing ever writes (every writerof this field uses
"prompts"). This crashed the v1 sync path with aKeyError at the first rollout call. Fixed to read
micro_batch["prompts"]instead; this was required to unblock v1 testing for this PR.