Enable optional batched Muon NS iteration#79482
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
CI报告基于以下代码生成(30分钟更新一次): 1 Required任务 : 46/48 通过当前 required 任务无失败,CI 仍在运行中:失败 0,运行中 2,等待中 0。
2 失败详情无 |
8a6e662 to
756ed0b
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查新提交。分组 key 相关问题已修复;split_concat_func 的 batch 输入契约问题仍未完全解决,我已在原 inline 线程补充了当前代码和测试覆盖的证据。请继续修复后再提交新的 commit。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #79482 +/- ##
==========================================
Coverage ? 93.50%
==========================================
Files ? 1
Lines ? 77
Branches ? 0
==========================================
Hits ? 72
Misses ? 5
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
756ed0b to
c4227a4
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。分组 key 问题和标题已处理,但 split_concat_func 的 batch 输入契约问题仍在;我已在原线程补充了具体复查说明。当前还需要继续修复后再合入。
|
/re-run all-failed |
c4227a4 to
964076b
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查当前提交,仍有阻塞问题;详细说明已经写在 inline review comments 和原线程里。当前这版还不能合入。
| # Group parameters by split_concat_func and shape, then update | ||
| # each group in one batched call. | ||
| muon_groups = defaultdict(list) |
There was a problem hiding this comment.
优先级:P1
处理要求:请针对该评论修复并提交新的 commit。
这里把 Muon 参数无条件分组批处理了:上一版的 MUON_ENABLE_BATCH 开关和逐参数 fallback 已被删除,当前仓库里也没有任何 MUON_ENABLE_BATCH 读取,test_muon_sharding_batched_update_group 也随之移除。但 PR 描述仍说明 group NS iteration 由该环境变量控制,因此未设置环境变量的现有训练现在也会默认走新的 batch 路径,破坏原来的可回退行为,并会放大已有 split_concat_func 输入契约问题。
请恢复开关语义:默认继续逐参数更新,仅在 MUON_ENABLE_BATCH=1 时进入分组 batch 路径。实现形态可以类似:
if not MUON_ENABLE_BATCH:
for param, grad in muon_params:
self._muon_update_group([(param, grad)], ...)
else:
# existing grouping logicThere was a problem hiding this comment.
目前只保留了_muon_update_group函数,删除了_muon_update函数。因为所有case都可以等价替换。不管有没有slice_func,shape是2D还是3D。
There was a problem hiding this comment.
这个判断我不同意。当前实现里,多个同 shape 的参数会先被合并成 batched_matrix,再交给 split_concat_func;而仓库里的 _qkv_sep / _ffn_split 仍然是按单个 2D 矩阵、axis=1 写的,不能直接把 grouped batch 当成等价输入。
所以删掉 _muon_update 并不自动成立“所有 case 都可等价替换”。如果要保留这个结论,至少需要把带 split_concat_func 的 group 改成 batch-aware 接口,或者对这类 group 保留逐参数路径,并补上多个同 shape、同 slice 函数的回归测试。
There was a problem hiding this comment.
这个判断我不同意。当前 head 里还有一个更直接的语义问题:lr_ratio/effective_lr 和最后的 subtract_ 已经被移到 for 外面,导致 group 里的前几个参数只做了前面的 decay,真正的 Muon step 只会落到最后一个参数上。这个状态下,哪怕不考虑 split_concat_func,_muon_update_group 也不能和逐参数路径视为等价。
我已经在对应代码行补了 inline 评论,请先把这个循环结构修正,再讨论是否可以删除 _muon_update。split_concat_func 的 2D 契约问题也仍然需要单独处理。
PaddlePaddle-bot
left a comment
There was a problem hiding this comment.
🤖 Paddle-CI-Agent | pr_review |
2026-07-16 23:33:38
📋 Review 摘要
PR 概述:为 Muon optimizer 引入按形状和 slice 函数聚合的 batched NS 更新路径。
变更范围:python/paddle/optimizer/muon.py、Muon sharding 分布式测试。
影响面 Tag:Performance Optimization Distributed Strategy User Experience
问题
| 级别 | 文件 | 概述 |
|---|---|---|
| 🔴 Bug | python/paddle/optimizer/muon.py:728 |
MUON_ENABLE_BATCH 未接入,分组 NS 无条件启用 |
历史 Findings 修复情况
| Finding | 问题 | 状态 |
|---|---|---|
| F1 | split_concat_func 接收分组后的 batched_matrix |
|
| F2 | 测试未对比 batch path 与逐参数旧路径 |
📝 PR 规范检查
标题缺少官方 Tag,描述结构和“是否引起精度变化”字段符合模板。
标题建议(可直接复制):
[Performance Optimization] Enable batched Muon group NS update
总体评价
当前分组实现和测试都已扩展,但描述里的 opt-in 开关没有落到代码,导致默认行为被直接切换;这个需要先补上 gating 和对应回归测试。历史 F1/F2 我检查后仍未看到修复证据,已在下方状态块标记。
|
|
||
| # Group parameters by split_concat_func and shape, then update | ||
| # each group in one batched call. | ||
| muon_groups = defaultdict(list) |
There was a problem hiding this comment.
🔴 Bug MUON_ENABLE_BATCH 没有接入,分组 NS 现在会无条件启用。
PR 描述说 group NS iteration 由 MUON_ENABLE_BATCH 控制,但当前文件全仓搜索不到这个环境变量;这里总是构建 muon_groups,随后所有 Muon 参数都会进入 _muon_update_group。这会把本应可选的优化变成默认行为,用户无法关闭新路径,也无法在 split 函数或数值回归时回退到逐参数更新。
建议修复方式:在模块加载时读取 MUON_ENABLE_BATCH,只有为 1 时才按 (split_concat_func, shape) 聚合;未开启时按 muon_params 顺序逐个调用 _muon_update_group([(param, grad)], ...),并补一组 env 未开启时不聚合的测试。
There was a problem hiding this comment.
目前只保留了_muon_update_group函数,删除了_muon_update函数。因为所有case都可以等价替换。不管有没有slice_func,shape是2D还是3D。
|
/re-run all-failed |
|
/re-run all-failed |
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。当前仍有阻塞问题,新的代码行级问题已写在 inline review comment,关于 _muon_update_group 等价性的回复也已补到原线程。请继续修复后再提交新的 commit。
|
|
||
| final_step = orthogonal_update * lr | ||
|
|
||
| lr_ratio = 1.0 if self._lr_ratio is None else self._lr_ratio(param) |
There was a problem hiding this comment.
优先级:P1
处理要求:请针对该评论修复并提交新的 commit。
这里 lr_ratio/effective_lr 和后面的减步逻辑已经跑到 for i, (param, _) in enumerate(group_params_grads): 外面了。按当前缩进,循环里的每个参数只会执行前面那段基于 lr 的 decay / final_step,而真正带 effective_lr 的更新只会作用到最后一个参数;这会让 group 内参数更新不一致,甚至把最后一个参数更新两次。即使不考虑 split_concat_func,这也不是和逐参数路径等价的实现。
请把 lr_ratio/effective_lr、decay 和 subtract_ 都放回每个参数的循环内部,形态应类似:
for i, (param, _) in enumerate(group_params_grads):
orthogonal_update = orthogonal_updates[i]
find_master = find_master_list[i]
master_weight = self._master_weights[param.name] if find_master else None
lr_ratio = 1.0 if self._lr_ratio is None else self._lr_ratio(param)
effective_lr = lr * lr_ratio
if with_decay and weight_decay > 0:
# scale current param/master_weight with effective_lr
...
final_step = orthogonal_update * effective_lr
# subtract final_step from the current param/master_weight
...There was a problem hiding this comment.
已复查当前 head,这条已经修复:lr_ratio/effective_lr、weight decay 和最终 subtract_ 都已经回到 for i, (param, _) in enumerate(group_params_grads): 循环内部,因此本线程的问题不再阻塞。
剩余阻塞项仍是另外两个独立问题:split_concat_func 的 batch 输入契约,以及 MUON_ENABLE_BATCH 开关未接入;我已在对应线程和最新 review 里说明。
| # Group parameters by split_concat_func and shape, then update | ||
| # each group in one batched call. | ||
| muon_groups = defaultdict(list) |
There was a problem hiding this comment.
这个判断我不同意。当前 head 里还有一个更直接的语义问题:lr_ratio/effective_lr 和最后的 subtract_ 已经被移到 for 外面,导致 group 里的前几个参数只做了前面的 decay,真正的 Muon step 只会落到最后一个参数上。这个状态下,哪怕不考虑 split_concat_func,_muon_update_group 也不能和逐参数路径视为等价。
我已经在对应代码行补了 inline 评论,请先把这个循环结构修正,再讨论是否可以删除 _muon_update。split_concat_func 的 2D 契约问题也仍然需要单独处理。
36b2604 to
f447b4f
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。lr_ratio/effective_lr 的循环位置问题已经修复;没有新增 inline 评论,我已在已有 split_concat_func 线程补充当前 head 的证据。
当前仍有两个 P1 阻塞项未完成:split_concat_func 仍直接接收 batched_matrix,并且 MUON_ENABLE_BATCH 仍未接入、分组路径仍默认启用。请按已有线程修复并提交新的 commit 后再复查。
|
/re-run all-failed |
2 similar comments
|
/re-run all-failed |
|
/re-run all-failed |
PR Category
Performance Optimization
PR Types
Performance
Description
Controlled by the
MUON_ENABLE_BATCHenvironment variable to enable group NS iteration.When group NS iteration is enabled, parameters with identical shapes and slice functions are grouped together and processed in a single batch NS iteration, reducing optimizer computation time.
是否引起精度变化
否