Repository navigation
fix(megatron): repack oversized KK microbatches - #419
DrRyanHuang wants to merge 1 commit into
Conversation
# 🐛 Bug Fix ## Fix dynamic microbatch packing - Return actual sample-index groups from First-Fit packing - Fall back to First-Fit when KK groups exceed the token budget - Split fallback groups to match DP/VPP microbatch counts
|
Thanks for contributing to Relax, @DrRyanHuang! 感谢你为 Relax 做出贡献! Contribution guide / 贡献指南Describe the problem, your changes, and how you validated them. Keep each PR focused and run 请说明问题、改动和验证方式,保持 PR 聚焦,并在提交前运行 CI commands / CI 指令
Put one command on the first line of a new PR comment. Rerun/cancel require PR authorship or repository write access. 在新 PR 评论的首行写一条指令。PR 作者或有仓库写权限的贡献者可以重跑、取消 CI。 |
Nyanpasu 审查看板审查状态: 💬 已完成 · 有补充意见 审查版本: 11edf3c 目标分支: main 审查完成,结论为 Comment:打包回退机制本身经验证正确(计数等价 3000 组、回退正确性 6000 组随机用例),无阻塞问题;提出 1 个 P2 测试缺口(F1)与 1 个 P3 非行级跟进建议(F2,SFT 路径同样缺少 KK 容量上限)。
审查发现待处理
已解决或已取代
提交范围 · 接收 2 · 建议移出 0 · 待确认 0接收 2 个文件 · 建议移出 0 个文件 · 待确认 0 个文件。移出与待确认部分暂停深审,不代表审查通过。
精简审查与验证依据
生产代码的必要性与替代方案
测试的必要性与替代方案
Powered by Nyanpasu with glm-5.3-flash max, please check the suggestions carefully.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rai-studio-bot
left a comment
There was a problem hiding this comment.
审查结论
本 PR 的修复经验证正确:重构后的 get_minimum_num_micro_batch_size 与原实现计数逐例一致(3000 组随机用例);对真实的 KK 分组与新的 First-Fit 回退(含按需拆包)做了 6000 组随机用例模拟,在 DP/VPP 包数对齐不变量下,回退后每个样本恰好覆盖一次、多样本分包均不超过 token 预算、包数与 dummy offset 均正确。现有调用方 relax/backends/megatron/actor.py:1461 的计数语义不受影响,pre-commit 全部通过。
看板(范围结论与验证依据):#419 (comment)
-
P3 · 非行级:问题位于本 PR 未改动的文件,无法挂到 diff 行 · SFT 预打包路径同样缺少 KK 容量上限relax/backends/megatron/actor.py:1462(以及:1294的 DP 对齐重分包)同样先以 First-Fit 计数、再用get_seqlen_balanced_partitions分组,且没有容量检查,与本 PR 修复前的 RL 路径模式相同。若 SFT 的 pack 长度可能严重不均,会有同样的超预算风险。本 PR 明确限定 RL 路径,此点留作后续跟进即可。
| if any(sum(seqlens[index] for index in partition) > capacity for partition in partitions): | ||
| # KK balances load without a capacity constraint. Only split the | ||
| # first-fit bins when DP/VPP require more micro-batches. | ||
| partitions = get_first_fit_partitions(seqlens, capacity) | ||
| while len(partitions) < real_partition_count: | ||
| partition = max(partitions, key=len) | ||
| partitions.append([partition.pop()]) |
There was a problem hiding this comment.
回退逻辑本身经验证是正确的:用真实的 get_first_fit_partitions 与 get_seqlen_balanced_partitions 加上本函数原文随机模拟 6000 组用例(含 DP/VPP 包数对齐不变量),回退后每个样本恰好覆盖一次、多样本分包均不超过 capacity、包数与 dummy offset 均正确;get_minimum_num_micro_batch_size 重构前后计数也逐例一致。
但目前没有测试保护这条路径:tests/backends/megatron/test_data_vpp.py 只覆盖 use_dynamic_batch_size=False,tests/utils/data/test_seqlen_balancing.py 只测 KK 分组。这段回退分支与 get_first_fit_partitions 都是纯函数,不依赖 GPU/分布式即可直接测试。建议补一个回归测试,至少覆盖:KK 超预算触发回退、回退后各分包不超过 capacity、样本覆盖完整、len(partitions) < real_partition_count 时按需拆包到目标包数。否则后续重构可能悄悄退回超预算打包,重新引入 OOM 风险。


What
修复普通 RL 动态分包中,KK 重新组合样本后 microbatch 可能超过 token 预算的问题。
Why
开启
--use-dynamic-batch-size后,旧代码先用 First-Fit 计算包数,再经 DP/VPP 对齐,最后由 KK 生成实际分组。问题是 First-Fit 只返回包数,没有保留实际分组;KK 只平衡负载,不限制每包的 token 总量。因此,即使包数正确,重新组合后的 microbatch 仍可能超预算,增加 OOM 风险。
How
参考 slime 默认动态 token 打包的 First-Fit 分组、拆包对齐和 KK 按整包分配到 DP。本 PR 保留 Relax 原有的合法 KK 分组,仅在超预算时回退。