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.

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

[Fluid] move ftrl to phi #56270

Closed
wants to merge 48 commits into from
Closed

Conversation

enkilee
Copy link
Contributor

@enkilee enkilee commented Aug 14, 2023

PR types

Others

PR changes

Others

Description

move ftrl to phi

@paddle-bot
Copy link

paddle-bot bot commented Aug 14, 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 Aug 14, 2023
@luotao1 luotao1 added the HappyOpenSource Pro 进阶版快乐开源活动,更具挑战性的任务 label Aug 14, 2023
@enkilee
Copy link
Contributor Author

enkilee commented Aug 28, 2023

@huangjiyi 佬能否有空帮忙看下,哪里写的有问题。CI的错误看不懂

@huangjiyi
Copy link
Member

huangjiyi commented Aug 28, 2023

暂时看不出问题在哪,但是报错的信息和维度相关,我帮你调试一下

@enkilee
Copy link
Contributor Author

enkilee commented Aug 28, 2023

暂时看不出问题在哪,但是报错的信息和维度相关,我帮你调试一下

谢谢

@huangjiyi
Copy link
Member

op = Operator(
"ftrl",
Param='Param',
Grad='Grad',
ParamOut='Param',
SquaredAccumulator='SquaredAccumulator',
SquaredAccumOut='SquaredAccumulator',
LinearAccumulator='LinearAccumulator',
LinearAccumOut='LinearAccumulator',
LearningRate='LearningRate',
l1=l1,
l2=l2,
lr_power=lr_power,
)

因为 ftrl_op 存在输入可能是 DenseTensorSelectedRows 2 种情况,你在 PR 里把这 2 种情况拆分成了 2 个 Kernel 没问题,但是单测里测试 TestSparseFTRLOp 中指定的 op 还是 ftrl,所以这里用到的 Kernel 还是 DenseTensor 情况的 FTRLOpKernel,没有跑到 FTRLOpSparseKernel 导致 CI 里的报错

按我的理解要解决上面这个问题的话需要另外实现一个 op ftrl_sparse(因为目前是只实现了 ftrl_sparse kernel 没有实现 ftrl_sparse op),然后把单测 TestSparseFTRLOp 中指定的 op 改为 ftrl_sparse,不知道我的理解对不对,可以请 @GhostScreaming 帮忙看看

Copy link
Contributor

Choose a reason for hiding this comment

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

参考paddle/phi/ops/compat/adam_sig.cc,添加一个ftrl_op_sig.cc。现在输入的类型可能为DenseTensor或者SelectedRows,op无法确定应该用FTRLKernel还是FTRLSparseKernel,目前应该是都匹配到了FTRLKernel。在本地跑test_ftrl_op.py,把Sparse相关的单测注释掉,是不会报Dim相关的错误的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

收到

Copy link
Contributor Author

Choose a reason for hiding this comment

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

那 ops.yaml 需要把 ftrl 加上吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

目前看着只有静态图用了ftrl这个op,可以先不加

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我参考adam_sig.cc改了,还是在test那报错。不懂错误提示的是什么

Copy link
Contributor

Choose a reason for hiding this comment

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

删除空行。可以在本地先跑通这个单测。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

收到

@paddle-ci-bot
Copy link

paddle-ci-bot bot commented Sep 16, 2023

Sorry to inform you that 697e221's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

@enkilee enkilee closed this Dec 19, 2023
@enkilee enkilee deleted the move-ftrl-to-phi branch December 19, 2023 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers HappyOpenSource Pro 进阶版快乐开源活动,更具挑战性的任务
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants