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

【Hackathon No.67】remove reference to operator.h in phi [part 1] #50624

Merged
merged 4 commits into from
Mar 9, 2023
Merged

【Hackathon No.67】remove reference to operator.h in phi [part 1] #50624

merged 4 commits into from
Mar 9, 2023

Conversation

RedContritio
Copy link
Contributor

@RedContritio RedContritio commented Feb 17, 2023

PR types

Others

PR changes

Others

Describe

由于此前 <fluid/framework/operator.h> 和 <fluid/framework/phi_utils.h> 存在循环依赖,且 phi_utils.h 存在大量对 fluild 的依赖,因此此前的很多解耦合功能只是将引用放进了 phi_utils.h 中,没有真实解决依赖问题。

直接依赖的地方分别是 paddle/phi/kernels/funcs/math_function.hpaddle/phi/kernels/funcs/blas/blas.h

本 pr 解决 math_functions.h

先将 operator.h 的所有依赖展开,然后将展开项中的 phi_utils.h 展开,得到列表如下:

// operator.h
#include "paddle/fluid/framework/attribute.h"
#include "paddle/fluid/framework/block_desc.h"
#include "paddle/fluid/framework/convert_utils.h"
#include "paddle/fluid/framework/lod_tensor.h"
#include "paddle/fluid/framework/op_info.h"
#include "paddle/fluid/framework/op_kernel_type.h"
#include "paddle/fluid/framework/phi_utils.h"
#include "paddle/fluid/framework/scope.h"
#include "paddle/fluid/framework/selected_rows_utils.h"
#include "paddle/fluid/framework/unused_var_check.h"
#include "paddle/fluid/memory/malloc.h"

#include "paddle/phi/core/compat/arg_map_context.h"
#include "paddle/phi/core/compat/op_utils.h"
#include "paddle/phi/core/kernel_context.h"
#include "paddle/phi/core/kernel_factory.h"
#include "paddle/utils/flat_hash_map.h"

// phi_utils.h
#include "paddle/fluid/framework/framework.pb.h"
#include "paddle/fluid/framework/op_kernel_type.h"
#include "paddle/fluid/framework/operator.h" // 循环依赖
#include "paddle/fluid/framework/tensor.h"
#include "paddle/fluid/framework/variable.h"
#include "paddle/fluid/platform/macros.h"
#include "paddle/fluid/platform/place.h"
#include "paddle/phi/api/lib/utils/tensor_utils.h"
#include "paddle/phi/common/backend.h"
#include "paddle/phi/core/compat/arg_map_context.h"
#include "paddle/phi/core/kernel_factory.h"
#include "paddle/utils/flat_hash_map.h"
#include "paddle/utils/small_vector.h"

#ifdef PADDLE_WITH_XPU
#include "paddle/fluid/platform/device/xpu/xpu_op_list.h"
#endif
#ifdef PADDLE_WITH_CUSTOM_DEVICE
#include "paddle/phi/backends/custom/custom_device_op_list.h"
#endif

从列表中删除 operator.h,取交集,随后逐个将能替代的全部替换成 phi 内的头文件即可。

(也就是说,每次引入 operator.h,都会引入上面这个头文件集合)

  1. 展开 operator.h 引入的依赖,并放到实际使用的地方
  2. 添加 VisitPlacephi/core/utils/visit_place.h
  3. 替换掉改动文件中的 fluid/framework.proto 中定义的类型及方法,以避免引入额外的 fluid 头文件

@paddle-bot
Copy link

paddle-bot bot commented Feb 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 Feb 17, 2023
@RedContritio
Copy link
Contributor Author

RedContritio commented Feb 17, 2023

这里有一个类型引用 paddle::framework::ExecutionContext,该类型仅声明在 fluid/framework/operator.h 中。

#include "paddle/fluid/framework/operator.h"

template <typename DeviceContext, typename T>
inline BlasT<DeviceContext, T> GetBlas(
const paddle::framework::ExecutionContext& exe_ctx) {
return BlasT<DeviceContext, T>(
exe_ctx.template device_context<DeviceContext>());
}

我是否应该创建 phi/core/operator.h (及 phi/core/operator.cc)以定义该类型,并在原有的 fluid/framework/operator.h 中使用 using 来导出 phi::ExecutionContext 使成为 paddle::framework::ExecutionContext
或者考虑将 operator.h (及 .cc)整个移动到 phi 中?

@YuanRisheng

@RedContritio RedContritio changed the title [PHI decoupling] remove reference to fluid/framework/operator.h in phi 【Hackathon No.67】[PHI decoupling] remove reference to operator.h in phi Feb 22, 2023
@RedContritio RedContritio changed the title 【Hackathon No.67】[PHI decoupling] remove reference to operator.h in phi 【Hackathon No.67】remove reference to operator.h in phi [part 1] Feb 23, 2023
@RedContritio
Copy link
Contributor Author

@YuanRisheng 辛苦先 review 这一部分的,另一部分对这部分有依赖,且改动有点多

paddle/phi/kernels/funcs/math_function.cc Outdated Show resolved Hide resolved
paddle/phi/kernels/funcs/math_function.h Outdated Show resolved Hide resolved
paddle/phi/kernels/funcs/math_function_impl.h Outdated Show resolved Hide resolved
paddle/phi/kernels/impl/activation_grad_impl.h Outdated Show resolved Hide resolved
paddle/phi/kernels/set_value_grad_kernel.h Outdated Show resolved Hide resolved
paddle/fluid/framework/data_layout_transform.h Outdated Show resolved Hide resolved
@luotao1
Copy link
Contributor

luotao1 commented Feb 27, 2023

辛苦先 review 这一部分的,另一部分对这部分有依赖,且改动有点多

好的,可以等这个 PR 合入后尽快提交下一个 PR,奖金以最后完成的 PR 为准。

@luotao1
Copy link
Contributor

luotao1 commented Mar 2, 2023

需要按照黑客松流程在总issue下回复状态:

paddle/phi/kernels/funcs/math_function.h Outdated Show resolved Hide resolved
paddle/phi/common/place.h Outdated Show resolved Hide resolved
@RedContritio
Copy link
Contributor Author

RedContritio commented Mar 4, 2023

@xiegegege 问一下这个 CI 没过是因为啥

出错关键字可以搜索 "Traceback"

2023-03-04 12:25:14 Error: Can not import paddle core while this file exists: /usr/local/python3.7.0/lib/python3.7/site-packages/paddle/fluid/libpaddle.so
2023-03-04 12:25:14 Traceback (most recent call last):
2023-03-04 12:25:14 File "", line 1, in
2023-03-04 12:25:14 File "/usr/local/python3.7.0/lib/python3.7/site-packages/paddle/init.py", line 27, in
2023-03-04 12:25:14 from .framework import monkey_patch_variable
2023-03-04 12:25:14 File "/usr/local/python3.7.0/lib/python3.7/site-packages/paddle/framework/init.py", line 17, in
2023-03-04 12:25:14 from . import random # noqa: F401
2023-03-04 12:25:14 File "/usr/local/python3.7.0/lib/python3.7/site-packages/paddle/framework/random.py", line 16, in
2023-03-04 12:25:14 import paddle.fluid as fluid
2023-03-04 12:25:14 File "/usr/local/python3.7.0/lib/python3.7/site-packages/paddle/fluid/init.py", line 36, in
2023-03-04 12:25:14 from . import framework
2023-03-04 12:25:14 File "/usr/local/python3.7.0/lib/python3.7/site-packages/paddle/fluid/framework.py", line 34, in
2023-03-04 12:25:14 from . import core
2023-03-04 12:25:14 File "/usr/local/python3.7.0/lib/python3.7/site-packages/paddle/fluid/core.py", line 355, in
2023-03-04 12:25:14 raise e
2023-03-04 12:25:14 File "/usr/local/python3.7.0/lib/python3.7/site-packages/paddle/fluid/core.py", line 269, in
2023-03-04 12:25:14 from . import libpaddle
2023-03-04 12:25:14 ImportError: libpaddle_full_api_shared.so: cannot open shared object file: No such file or directory

paddle/phi/core/operator.h Outdated Show resolved Hide resolved
paddle/phi/kernels/impl/activation_impl.h Outdated Show resolved Hide resolved
paddle/phi/kernels/set_value_grad_kernel.h Outdated Show resolved Hide resolved
@luotao1
Copy link
Contributor

luotao1 commented Mar 6, 2023

https://www.paddlepaddle.org.cn/documentation/docs/zh/develop/dev_guides/code_contributing_path_cn.html#span-id-caution1-pr-span
image
建议:每次提交时,保持尽量少的 commit。可以通过git rebase -i HEAD~3将最新的 3 个 commit 合并成一个(可以根据实际情况修改该数值),再 Push 到远程仓库

@RedContritio 建议提交的时候,将本地的commit压缩一下,便于审核人看,现在commit太多了。

@RedContritio
Copy link
Contributor Author

https://www.paddlepaddle.org.cn/documentation/docs/zh/develop/dev_guides/code_contributing_path_cn.html#span-id-caution1-pr-span image 建议:每次提交时,保持尽量少的 commit。可以通过git rebase -i HEAD~3将最新的 3 个 commit 合并成一个(可以根据实际情况修改该数值),再 Push 到远程仓库

@RedContritio 建议提交的时候,将本地的commit压缩一下,便于审核人看,现在commit太多了。

了解了,已经在(今日中午)最新的 develop 上重建了 PR 分支,并根据功能进行 commit,最终保留三个 commit。

@RedContritio
Copy link
Contributor Author

RedContritio commented Mar 7, 2023

参考此前的 review 意见重新整理了一遍,剩下几个(没过的) CI 感觉是合理范围。

  • APPROVAL:20文件
  • Coverage:迁移常态覆盖率不够
  • Model-benchmark:感觉玄学,重跑过但是没差
  • OP-benchmark:超时两次了,同玄学

@luotao1
Copy link
Contributor

luotao1 commented Mar 8, 2023

Model-benchmark:感觉玄学,重跑过但是没差
OP-benchmark:超时两次了,同玄学

这两个不用管,等 @YuanRisheng 审核通过后,可以申请豁免

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@YuanRisheng YuanRisheng merged commit 9ffdb2b into PaddlePaddle:develop Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants