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

[Fluid] move lars_momentum to phi #55798

Merged
merged 28 commits into from
Aug 29, 2023
Merged

[Fluid] move lars_momentum to phi #55798

merged 28 commits into from
Aug 29, 2023

Conversation

gouzil
Copy link
Member

@gouzil gouzil commented Jul 29, 2023

PR types

Others

PR changes

Others

Description

将 lars_momentum 迁移到 PHI 下

相关issue：

@paddle-bot
Copy link

paddle-bot bot commented Jul 29, 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 Jul 29, 2023
@gouzil gouzil mentioned this pull request Jul 29, 2023
Closed
@luotao1 luotao1 added the HappyOpenSource 快乐开源活动issue与PR label Jul 31, 2023
gouzil
gouzil commented Jul 31, 2023
View reviewed changes
paddle/phi/kernels/gpu/lars_momentum_kernel.cu Outdated
// {LARS_BLOCK_SIZE << 1}, cuda_ctx);
phi::DenseTensor tmp_buffer_t;
tmp_buffer_t.Resize({LARS_BLOCK_SIZE << 1});
MT* p_buffer = dev_ctx.template Alloc<MT>(tmp_buffer_t);
Copy link
Member Author

Choose a reason for hiding this comment

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

需要研发大哥帮忙看看

Copy link

Choose a reason for hiding this comment

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

需要研发大哥帮忙看看

Alloc应该传指针

Copy link
Member Author

Choose a reason for hiding this comment

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

emmm, 我的意思是AllocateTmpTensor在phi下是这么用的嘛

Copy link

Choose a reason for hiding this comment

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

emmm, 我的意思是AllocateTmpTensor在phi下是这么用的嘛

AllocateTmpTensor实现的逻辑就是Resize+Alloc，按照你当前用法就可以，phi下ctx不一样了，不支持直接调用AllocateTmpTensor

@gouzil
Copy link
Member Author

gouzil commented Aug 7, 2023 •
edited
Loading

@hitywt 求助研发大哥PR-CI-Windows-OPENBLAS

@luotao1 luotao1 added HappyOpenSource Pro 进阶版快乐开源活动，更具挑战性的任务 and removed HappyOpenSource 快乐开源活动issue与PR labels Aug 7, 2023
@GhostScreaming
Copy link
Contributor

GhostScreaming commented Aug 10, 2023 •
edited
Loading

看一下PR-CI-Windows-OPENBLAS的报错，全量日志的15598行。看着是重载冲突，显式指定一下g的类型试试？
2023-08-07 12:27:29 ..\paddle\phi\kernels\cpu\lars_momentum_kernel.cc(52): error C2666: 'Eigen::operator *': 6 overloads have similar conversions，

@gouzil
Copy link
Member Author

gouzil commented Aug 11, 2023

看一下PR-CI-Windows-OPENBLAS的报错，全量日志的15598行。看着是重载冲突，显式指定一下g的类型试试？ 2023-08-07 12:27:29 ..\paddle\phi\kernels\cpu\lars_momentum_kernel.cc(52): error C2666: 'Eigen::operator *': 6 overloads have similar conversions，

看样子是windows下不支持和float相乘了, 但是直接用double会导致test_momentum_op单测失败

@gouzil gouzil requested a review from hitywt August 21, 2023 04:37
@gouzil
Copy link
Member Author

gouzil commented Aug 24, 2023

@hitywt 麻烦 review 一下

hitywt
hitywt reviewed Aug 28, 2023
View reviewed changes
test/legacy_test/test_momentum_op.py
@@ -312,7 +312,7 @@ def setUp(self):

def test_check_output(self):
paddle.enable_static()
self.check_output()
self.check_output(check_dygraph=False)
Copy link

Choose a reason for hiding this comment

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

这里为什么要改成False？

Copy link
Member Author

Choose a reason for hiding this comment

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

此算子没有动态图模式(在python中没找到相关api)，C++算子仅用于静态图。lars_momentum单测原本通过paddle.enable_static()设置只测试静态图。本PR将算子迁移到phi后，触发单测框架动态图测试拦截，因而在TestLarsMomentumOp中通过设置check_dygraph=False保持静态图测试。

hitywt
hitywt approved these changes Aug 28, 2023
View reviewed changes
Copy link

@hitywt hitywt 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 b0c2ee2 into PaddlePaddle:develop Aug 29, 2023
@luotao1
Copy link
Contributor

luotao1 commented Aug 29, 2023

@gouzil lars_momentum 需要补充 xpu kernel 和 infershape 么？

@gouzil
Copy link
Member Author

gouzil commented Aug 29, 2023

@gouzil lars_momentum 需要补充 xpu kernel 和 infershape 么？

需要的infershape在 #56749, xpu 在 #56751

BeingGod pushed a commit to BeingGod/Paddle that referenced this pull request Sep 9, 2023
@gouzil @BeingGod
[Fluid] move lars_momentum to phi (PaddlePaddle#55798)
091669f 
* [Fluid] move lars_momentum to phi

* add sig

* fix optional Output

* off check_dygraph

* fix input

* fix operator[]

* fix

* try fix AllocateTmpTensor

* fix

* fix type

* Update paddle/phi/kernels/gpu/lars_momentum_kernel.cu

* fix type

* rollback

* Add Registration

* try fix win

* try fix win

* try use double

* try use operator *(float,const Derived &)

* try auto

* fix

* fix

* fix

* fix dtype

* fix type

* fix index
@gouzil gouzil deleted the move_lars_momentum2phi branch September 24, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luotao1 luotao1 luotao1 approved these changes

@hitywt hitywt hitywt approved these changes

Assignees

@hitywt hitywt

@luotao1 luotao1

@GhostScreaming GhostScreaming

Labels
contributor External developers HappyOpenSource Pro 进阶版快乐开源活动，更具挑战性的任务
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gouzil @GhostScreaming @luotao1 @hitywt