【Hackathon No.19】Implement ASGD optimizer #42431
Conversation
|
你的PR提交成功，感谢你对开源项目的贡献! |
paddle/utils/variant.h
Outdated
| #if defined(__GNUC__) && !defined(__clang__) && __GNUC__ >= 9 | ||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wdeprecated-copy" | ||
| #endif |
There was a problem hiding this comment.
本文件的改动已经单独提交了 PR：#42265 ，在本 PR 的 review 中可以忽略这些改动
| assert np.array_equal(asgd.averaged_parameters()[0].numpy(), np_neg_ones * 3.5) | ||
| asgd.step() | ||
| assert np.array_equal(model._w.numpy(), np_neg_ones * 5) | ||
| assert np.array_equal(asgd.averaged_parameters()[0].numpy(), np_neg_ones * 4) |
There was a problem hiding this comment.
已和 pytorch 对比，结果是一致的
| loss = model(x) | ||
| print(f'2: w grad before bw: {model._w.grad}') | ||
| loss.backward() | ||
| print(f'2: w grad: {model._w.grad}') |
There was a problem hiding this comment.
【需要帮助】
这里有很奇怪的问题。这个测试在本地是可以通过的，但在 CI 环境里，第二次 loss.backward() 前后 model._w.grad 均为 None（通过这些 print 语句看出的）
There was a problem hiding this comment.
CI里提示有CONFLICT，先同步一下最新的代码再试试吧
There was a problem hiding this comment.
conflict 是来自于我自己的这个 PR 被合并到 develop 了：#42265
There was a problem hiding this comment.
这个问题很奇怪。本地是完全正常的，但 ci 环境里 loss.backward() 后 grad 竟然是 None。
或者有什么方法强行设置 model._w 的 grad 也可以，我尝试过 model._w.grad = ... 但会报错，不知道有没有其他方式
There was a problem hiding this comment.
@tiancaishaonvjituizi 您可太热心了，这个咱们之前讨论过了，感谢建议，我们后面会看下有没有更好的方式去替代这部分自动生成声明的实现，结合编译效率来看的话，每个文件中都展开一编注册的代码确实也不太高效，这确实不是一种好的方式。
不过其实这个设计在跟您在这儿讨论的这批人加入paddle之前就是这样的了，#14413
这个我理解在当时也起到了降低开发成本的作用，也没有您说的那么严重，但当然这并不是一种好的方式我也认可。我们后来人在此基础上的开发和重构都要在时效和目标之间做平衡和取舍，在目前的体系下，这样的方式既能够减少大家的开发成本，也能满足快速迭代上线的目标。如果我们先对整个paddle编译的体系进行重构优化的话，当然也能避免这种写法，但在算子库重构启动的时候这个不是最高优目标，这不只是一个技术的问题，涉及到团队规划和管理的多方面因素。
总之感谢您的建议，无论是编译方式和编译效率优化，还是Python API的分支写法优化，我们最近都在高优推进了，希望理解，也希望再给我们一些耐心，paddle会变得更好，也希望您能给我们更多的输入，大家一起将paddle建设得更好
There was a problem hiding this comment.
用C++的宏定义的时候，确实要很小心。
@jzhang533 这和“使用宏的时候要小心”没有关系，可能是你还没有了解为什么这里换一下位置就会报错
There was a problem hiding this comment.
不过其实这个设计在跟您在这儿讨论的这批人加入paddle之前就是这样的了，#14413
这个我理解在当时也起到了降低开发成本的作用，也没有您说的那么严重，但当然这并不是一种好的方式我也认可。我们后来人在此基础上的开发和重构都要在时效和目标之间做平衡和取舍，在目前的体系下，这样的方式既能够减少大家的开发成本，也能满足快速迭代上线的目标。如果我们先对整个paddle编译的体系进行重构优化的话，当然也能避免这种写法，但在算子库重构启动的时候这个不是最高优目标，这不只是一个技术的问题，涉及到团队规划和管理的多方面因素。
@chenwhql 通俗的讲就是只想着赶紧做业务，忽视了对底层代码的维护和迭代，所以屎山代码越堆越大，直到某一天爆雷了，才意识到要花十倍的时间去弥补 😂 我可以理解，同时 “屎山代码越堆越大不是好事”也是不需要再专门说明原因的老生常谈。还是希望 paddle 团队多多自我审视，软件工程就是在和人“再苟一苟”的惰性斗争
也没有您说的那么严重
这其实是一个常见的误区，原因是
- 屎山代码的危害本身就是温水煮青蛙
- 熟悉了内情的内部开发者是难以切身体会到其中的问题的，因为他在潜意识里已经熟知了那些坑，所以一举一动都会不自觉地落在“舒适区”里
其实这样做有两个方面的问题：
- 现实问题：新加入 paddle 开发的开发者在这里非常容易出错（我自己就是例子），特别是领域专家级别的开发者。小白开发者反而会少出问题，因为小白开发者本身就倾向于复制粘贴，不在乎代码冗余度和可维护性。也就带来这样一种反最佳实践的现象：好的写法无法通过编译，小白写法反而可以。这也是为什么我说这么做 “反映了 ‘鼓励流水线式批量产出代码，不在意可维护性’ 的潜意识”。
- 代码品味问题：C++ 语言的复杂和难以解析是一个众所周知的事情，在这种情况下 paddle “头铁” 地用字符串匹配来解析 C++ 代码，在熟悉 C++ 的人的眼里，是一件大跌眼镜的事情，挺损害 paddle 的形象的
There was a problem hiding this comment.
关于5，如上所述，目前的构建体系中有大量的静态库，新增c++代码需要手动修改很多依赖，导致cmake和make速度较慢，后面的一个优化方向是只编译少数几个动态库，这样可以把依赖层级扁平化。
请问这一部分有后续的更新/详细计划吗?
|
请先通过CI噢~ |
|
PR格式检查通过，你的PR将接受Paddle专家以及开源社区的review，请及时关注PR动态。 |
|
@dingjiaweiww 单测在本地是可以通过的，但在 CI 环境里有难以理解的问题（loss.backward() 前后 model._w.grad 均为 None）。有没有 paddle 开发者可以帮助看一下呢 #42431 (comment) |
|
我来看看哈 |
@JiabinYang 😂 这个任务我弃疗了，ci 里那个问题不是所有的原因，另一个原因是我看到了 paddle 其它 optimizer 的单测，望而生畏，自问即使解决了 ci 的那个问题，也不一定能在期限内把单测写成那样，因为我对那一套（也许是旧式的）接口还不太了解，当时考虑到我同时还报名了几个其它任务，就把精力放在那些任务上面了。 |
别放弃 哈哈哈 你可以试试我这个PR合并进来应该可以解决你的问题：https://github.com/PaddlePaddle/Paddle/pull/42805， 这个问题产生的原因是因为我前面提到的我们正在做动态图的底层架构升级为了能极大的提升动态图的执行效率，而这期间会存在部分兼容性的问题，优化器作为需要把输出传入进行处理的算子我们通过这样一个白名单来维护。但是这个确实不太合理 我们也正在处理这个问题 |
|
Sorry to inform you that 46e5362's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
|
Since you haven't replied for more than a year, we have closed this issue/pr. |
|
很抱歉，经过我们的反复讨论，你的PR暂未达到合入标准，请阅读飞桨原生算子开发规范，你可以重新提交新的PR，我们先将此PR关闭，感谢你的贡献。 |
PR types
New features
PR changes
APIs
Describe
实现 ASGD Optimizer
Hackathon issue 链接：#40314
Hackathon RFC 链接：PaddlePaddle/community#68