[doc] feat: add performance tuning guide entry point - #487
Conversation
Consolidates the GPU-layout decision (colocated vs disaggregated reward pool vs multi-node) and links out to the existing rollout batching, MFU tuning, async reward, and profiler docs instead of duplicating them, plus an OOM/bottleneck troubleshooting checklist that spans all three stages.
There was a problem hiding this comment.
🟡 Changes recommended
A few documentation links/config references in the new entry-point page are misleading or ambiguous and should be corrected to keep navigation accurate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new performance-tuning entry-point page to the docs and wires it into the main docs index so readers have a single “decision order” + troubleshooting checklist for diffusion RL tuning.
Changes:
- Add
docs/perf/tuning_guide.mdas the new landing page for performance tuning guidance. - Link the new tuning guide into
docs/index.mdas the first entry under “Performance Tuning Guide”.
File summaries
| File | Description |
|---|---|
| docs/perf/tuning_guide.md | New entry-point page outlining tuning order and a consolidated troubleshooting checklist, linking out to existing detailed docs. |
| docs/index.md | Adds the new tuning guide to the “Performance Tuning Guide” toctree so it appears in the docs navigation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fix three issues flagged by Copilot review: the request-level batching link resolved to the contributor-guide anchor instead of rollout_batching.md, the MFU tuning-section link pointed at the page top instead of the actual section, and enable_resource_pool was missing its reward.reward_model. prefix.
|
Fixed all three in dd574b7:
|
SamitHuang
left a comment
SamitHuang
left a comment
There was a problem hiding this comment.
Summary
Good entry-point structure: profile first, then layout → rollout → actor, with a cross-stage checklist that links out instead of duplicating existing pages. Copilot's three link/prefix nits look fixed in dd574b7.
Five inline comments cover remaining factual issues (GPU layout framing, Hydra knob paths, diffusion vs KV wording, MFU > 1.0 causes, missing link to docs/algo/performance.md).
PR title: [doc] doc: … will fail tests/special_sanity/check_pr_title.py. Allowed types are feat, fix, refactor, chore, test — suggest [doc] feat: add performance tuning guide entry point.
Requesting changes on the inline items; happy to re-review once addressed.
…guide Splits colocated/disaggregated reward placement from multi-node scaling (the default multi-node recipe still colocates reward), names the engine_kwargs.vllm_omni Hydra paths for the batching knobs instead of the bare dataclass field, corrects the rollout OOM section to describe packed activation memory instead of an LLM KV cache, points the MFU > 1.0 troubleshooting at the two documented causes instead of the LoRA caveat, and links the throughput reference table. Co-authored-by: Claude Code
OnePunchMonk
changed the title
|
@SamitHuang made fixes |
|
@zhtmike, tagging you to request a review here. Thank you! |
There was a problem hiding this comment.
🟡 Changes recommended
Two configuration recommendations conflict with existing documented and tested behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
- rollout.max_num_seqs is the top-level engine concurrency knob in both step-wise and request-level batching, not a no-op field. - Split the update_actor/update_weights OOM entry into a forward/backward activation OOM case (fix: lower ppo_micro_batch_size_per_gpu) and a distinct optimizer/param-state OOM case (fix: offload ordering), since the previous single entry pointed forward/backward OOMs at the wrong first step.
|
@zhtmike @SamitHuang made changes |
|
looks good, some hyparameters are indeed not so obvious, like |
|
@knlnguyen1802 you may take a look |
knlnguyen1802
left a comment
knlnguyen1802
left a comment
There was a problem hiding this comment.
Can also mention about TransferQueue and v1 version of verl, which the main bottleneck is data transfer between Worker Group, it is very helpful for video generation model too
|
Now that the profile tools are quite mature, I think we can add a "skills" section to the profile. |
RL-Insight (verl-project#465) merged after this guide was written; point readers to it for live per-run metrics alongside the one-off profiler trace.
Summary
Addresses the "Supplement docs for performance tuning" 🙋 item from the Q3 roadmap (#97).
Adds
docs/perf/tuning_guide.mdas an entry point for the existing "Performance Tuning Guide" docs section, which today only listsdiffusion_mfu.mdandprofiler.md. The new page:rollout_batching.md,diffusion_mfu.md's "Tuning and Improving MFU" section,async_reward.md,multi_node_training.md) rather than duplicating their content.Wired into
docs/index.md's toctree as the first entry under "Performance Tuning Guide".Not a duplicate: checked open PRs for "performance"/"profiling docs" in title and found none addressing this checklist item.
Test plan
python3 tests/special_sanity/check_docs_time_info.pypasses (new page has a "Last updated" line)verl/SGLangcasing) — no matches outside allowed forms{ref}targets resolve to existing anchors (rollout_batching,request-level-batching,diffusion_mfu) and.md#anchorlinks follow the same pattern used elsewhere indocs/Drafted with AI assistance (Claude Code); I reviewed the content and links above before submitting.