Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Functionalize distributed_fused_lamb kernel #53896

Merged
merged 21 commits into from
May 23, 2023
Merged

Functionalize distributed_fused_lamb kernel #53896

merged 21 commits into from
May 23, 2023

Conversation

huangjiyi
Copy link
Member

@huangjiyi huangjiyi commented May 17, 2023 •
edited
Loading

PR types

Others

PR changes

Others

Description

  • Functionalize distributed_fused_lamb kernel
  • 解除了大部分 distributed_fused_lamb kernel 对 fluid 的依赖，还剩余小部分依赖不好处理，因为这个 Kernel 过于复杂，所以先提一个 PR，后续再提一个 PR 解除剩余的依赖然后移动到 phi 下
  • [PHI] Functionalization for Fluid kernel #52395

@paddle-bot
Copy link

paddle-bot bot commented May 17, 2023

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels May 17, 2023
@paddle-bot
Copy link

paddle-bot bot commented May 17, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@huangjiyi huangjiyi mentioned this pull request May 17, 2023
Closed
@luotao1 luotao1 added the HappyOpenSource 快乐开源活动issue与PR label May 18, 2023
@huangjiyi
Copy link
Member Author

@From00 ，帮忙 Review 一下 ～

From00
From00 reviewed May 18, 2023
View reviewed changes
Copy link
Contributor

@From00 From00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

此算子若依赖复杂，解耦困难，可以先保留在fluid目录下。

paddle/fluid/operators/optimizers/distributed_fused_lamb_op.cu Show resolved Hide resolved
paddle/fluid/operators/optimizers/distributed_fused_lamb_op.cu Outdated Show resolved Hide resolved
@huangjiyi
Copy link
Member Author

此算子若依赖复杂，解耦困难，可以先保留在fluid目录下。

若保留在 fluid 目录下的话，namespace 是不是也需要改回 paddle::operators

@From00
Copy link
Contributor

From00 commented May 18, 2023

此算子若依赖复杂，解耦困难，可以先保留在fluid目录下。

若保留在 fluid 目录下的话，namespace 是不是也需要改回 paddle::operators

不需要，只是代码文件先放置在fluid，其它行为都保持和在phi下一样。后续依赖解耦后，也是会将其迁移到phi。

@huangjiyi huangjiyi closed this May 18, 2023
@paddle-bot
Copy link

paddle-bot bot commented May 18, 2023

很抱歉，经过我们的反复讨论，你的PR暂未达到合入标准，请阅读飞桨原生算子开发规范，你可以重新提交新的PR，我们先将此PR关闭，感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle Custom Operator Design Doc). You can also submit an new one. Thank you.

@huangjiyi huangjiyi reopened this May 18, 2023
@huangjiyi
Copy link
Member Author

commet 误点成 close 了。。

@huangjiyi
Copy link
Member Author

不需要，只是代码文件先放置在fluid，其它行为都保持和在phi下一样。后续依赖解耦后，也是会将其迁移到phi。

剩下的依赖中不好解耦的是 paddle/fluid/platform/collective_helper.h 中的 NCCLCommContext，虽然 phi 下也有 NCCLCommContext，但是好像还缺少一些功能

@huangjiyi
Copy link
Member Author

@From00 ，帮忙再 Review 一下 ~

From00
From00 previously approved these changes May 19, 2023
View reviewed changes
Copy link
Contributor

@From00 From00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@huangjiyi
Copy link
Member Author

@From00 ，帮忙再 Review 一下 ~

From00
From00 approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@From00 From00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@luotao1 luotao1 merged commit 5f8e7d8 into PaddlePaddle:develop May 23, 2023
@huangjiyi huangjiyi deleted the move_distributed_fused_lamb branch January 9, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@From00 From00 From00 approved these changes

Assignees

@luotao1 luotao1

@From00 From00

Labels
contributor External developers HappyOpenSource 快乐开源活动issue与PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@huangjiyi @From00 @luotao1