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

[PHI decoupling] simplify "convert_utils.h" in fluid #48168

Merged
merged 13 commits into from
Nov 24, 2022

Conversation

huangjiyi
Copy link
Member

@huangjiyi huangjiyi commented Nov 19, 2022

PR types

Others

PR changes

Others

Describe

这个 PR 本打算清理之前 phi 中意外引入的一个 convert_utils.h 依赖同时移除 cudnn_desc.h 依赖,但发现 cudnn_desc.h 依赖了 enforce.h,这个比较麻烦,所以我稍微简化了一下 fluid 下的 convert_utils.h.

Changes:

  • paddle/phi/common/data_type.h 中已经存在了 Sizeof (获取 DataType 字节)以及 DataTypeToString 这两个函数,这与 convert_utils.h 中的 DataType2String(之前移动到了 phi 下) 和 DataTypeSize 的功能一致,所以移除了 convert_utils.h 中的两个函数,并替换相关调用。
  • convert_utils.h 中的 TransToPhiDataTypeTransToProtoVarType 与我们之前写的两个 map 映射的功能重复了,因为 convert_utils.h 中的两个函数考虑的更加全面,然后我将这个两个函数移动到 paddle/phi/core/utils/data_type.h" 中,并删除了原来写的 map, 同时替换相关调用。
  • 移除 convert_utils.cc,同时 convert_utils.h 中使用 using,本来想完全移除 convert_utils.h,但是依赖它的文件太多了。

@paddle-bot
Copy link

paddle-bot bot commented Nov 19, 2022

你的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-old paddle-bot-old bot added the contributor External developers label Nov 19, 2022
Comment on lines +33 to +35
inline proto::VarType::Type TransToProtoVarType(const DataType& dtype) {
return static_cast<proto::VarType::Type>(phi::TransToProtoVarType(dtype));
}
Copy link
Member Author

@huangjiyi huangjiyi Nov 20, 2022

Choose a reason for hiding this comment

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

添加这个函数而不是直接使用 using 是因为 phi 中的 TransToProtoVarType 返回的是 int 类型的值,如果将 TransToProtoVarType 的返回值赋给某个 proto::VarType::Type 类型的参数编译就会报错,因为不存在 int 到枚举类型的自动转换。

@huangjiyi huangjiyi changed the title [PHI decoupling] decouple "convert_utils.h" in fluid [PHI decoupling] simplify "convert_utils.h" in fluid Nov 20, 2022
@huangjiyi
Copy link
Member Author

@YuanRisheng @luotao1

inline std::string DataType2String(DataType dtype) {
// We can't depend on the fluid proto::VarType
// so we copy part of fluid proto::VarType here.
enum ProtoVarType {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里换个名字吧,Proto下的VarType是个比较广义的概念,它不仅包含了基本数据类型(int,float等),也包含了其他tensor的类型(Lod_tensor等),这里可以改名为ProtoDataType,这样就比较精准(当然你能想到更合适的名字也可以换一个),另外注释可以再详细一点,说明这里的枚举值与proto::VarType对应的,强调一下如果有修改的话,这里也需要同步进行修改

Copy link
Member Author

Choose a reason for hiding this comment

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

好的明白了

# or, the first one should depends on mkldnn
if(WITH_MKLDNN)
add_dependencies(convert_utils mkldnn)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

试试在11行后加

if(WITH_MKLDNN)
  set(convert_utils_deps ${convert_utils_deps} mkldnn)
endif()

去掉19-20行

@huangjiyi
Copy link
Member Author

@YuanRisheng ,可以再次 review 了

@huangjiyi
Copy link
Member Author

@luotao1 ,帮忙处理一下失败的 CI

@luotao1
Copy link
Contributor

luotao1 commented Nov 24, 2022

覆盖率流水线已豁免

@huangjiyi
Copy link
Member Author

@XiaoguangHu01 , request approval.

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

@luotao1 luotao1 merged commit de4310e into PaddlePaddle:develop Nov 24, 2022
@luotao1
Copy link
Contributor

luotao1 commented Nov 24, 2022

该 PR 合入后会导致 develop 上所有 build 流水线失败,已经在 #48347 修复了,可以再观察下。

@huangjiyi
Copy link
Member Author

该 PR 合入后会导致 develop 上所有 build 流水线失败,已经在 #48347 修复了,可以再观察下。

Sorry,给大家添麻烦了:cry:。

@huangjiyi
Copy link
Member Author

该 PR 合入后会导致 develop 上所有 build 流水线失败,已经在 #48347 修复了,可以再观察下。

@luotao1 ,我想请教一下为什么会出现这个问题,我看了之后的理解是没有 .cc 实现文件的头文件就不需要用 cc_library 构建库了,但是为什么会出现那个编译错误还是不太明白。

@luotao1
Copy link
Contributor

luotao1 commented Nov 25, 2022

Sorry,给大家添麻烦了

没事,合入后后续集成测试发现 Bug 的也有好多,是常态。

但是为什么会出现那个编译错误还是不太明白

因为我们 CI 上有编译缓存在,可能是编译缓存的问题。

@huangjiyi
Copy link
Member Author

因为我们 CI 上有编译缓存在,可能是编译缓存的问题。

明白了,感谢

@huangjiyi huangjiyi deleted the clean_convert_utils branch December 9, 2022 11:11
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.

4 participants