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

[PHI decoupling]decouple tensor_utils of TensorCopy #50264

Merged
merged 1 commit into from
Feb 14, 2023
Merged

[PHI decoupling]decouple tensor_utils of TensorCopy #50264

merged 1 commit into from
Feb 14, 2023

Conversation

engineer1109
Copy link
Contributor

PR types

Others

PR changes

Others

Describe

decouple tensor_utils of TensorCopy, and other functions

@paddle-bot
Copy link

paddle-bot bot commented Feb 6, 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 6, 2023
@engineer1109 engineer1109 force-pushed the tensorutil2 branch 3 times, most recently from b791a5f to 8235d06 Compare February 7, 2023 03:24
@engineer1109
Copy link
Contributor Author

部分是降低耦合程度
但是仍需要

#include "paddle/fluid/memory/memcpy.h"
#include "paddle/fluid/platform/place.h"

Approval需要豁免

@engineer1109 engineer1109 force-pushed the tensorutil2 branch 5 times, most recently from 61cf74c to 88d0fae Compare February 7, 2023 09:38
YuanRisheng
YuanRisheng reviewed Feb 7, 2023
View reviewed changes
paddle/phi/common/scalar.cc Outdated
@@ -14,9 +14,12 @@ limitations under the License. */

#include "paddle/phi/common/scalar.h"

#include "paddle/fluid/framework/tensor_util.h"
#include "paddle/fluid/platform/place.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

可以尝试删除这个头文件，如果遇到需要判断是哪种place（比如paddle::platform::is_same_place(tensor_in.place(), cpu_place)），可以参考这个代码：
image

paddle/phi/kernels/gpu/interpolate_kernel.cu Outdated
*out_size, paddle::platform::CPUPlace(), &sizes);
phi::DeviceContextPool& pool = phi::DeviceContextPool::Instance();
auto dev_ctx = pool.Get(out_size->place());
phi::Copy(*dev_ctx, *out_size, phi::CPUPlace(), true, &sizes);
Copy link
Contributor

Choose a reason for hiding this comment

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

这里看起来函数参数有传入dev_ctx，还需要再从pool中拿吗

paddle/phi/kernels/gpu/interpolate_kernel.cu Outdated
*out_size, paddle::platform::CPUPlace(), &sizes);
phi::DeviceContextPool& pool = phi::DeviceContextPool::Instance();
auto dev_ctx = pool.Get(out_size->place());
phi::Copy(*dev_ctx, *out_size, phi::CPUPlace(), true, &sizes);
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/kernels/gpu/interpolate_kernel.cu Outdated
*out_size, paddle::platform::CPUPlace(), &sizes);
phi::DeviceContextPool& pool = phi::DeviceContextPool::Instance();
auto dev_ctx = pool.Get(out_size->place());
phi::Copy(*dev_ctx, *out_size, phi::CPUPlace(), true, &sizes);
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/kernels/funcs/interpolate_function.h Outdated
@@ -96,15 +95,16 @@ inline std::vector<int> get_new_shape(
#ifdef PADDLE_WITH_XPU
if (tensor->place().GetType() == phi::AllocationType::XPU) {
DenseTensor temp;
paddle::framework::TensorCopySync(*tensor, phi::CPUPlace(), &temp);
phi::Copy<phi::CPUContext>(
phi::CPUContext(), *tensor, phi::CPUPlace(), true, &temp);
Copy link
Contributor

Choose a reason for hiding this comment

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

这里应该有Bug，创建了CPUContext临时对象

@engineer1109 engineer1109 force-pushed the tensorutil2 branch 9 times, most recently from 91e864f to 701888a Compare February 9, 2023 01:35
@engineer1109
Copy link
Contributor Author

修改超20个文件 PR-CI-APPROVAL 需要豁免
PR-CI-Coverage 覆盖率C++ 需要豁免
PR-CI-OP-benchmark 算子过多 一直超时，可能需要豁免

YuanRisheng
YuanRisheng reviewed Feb 9, 2023
View reviewed changes
paddle/phi/api/lib/data_transform.cc
@@ -188,7 +186,13 @@ inline phi::DenseTensor TransDataPlace(const phi::DenseTensor& tensor,
// But the embarrassment is that this solution this solution makes training
// slower.
phi::DenseTensor out;
paddle::framework::TensorCopySync(tensor, dst_place, &out);
phi::DeviceContext* dev_ctx;
if (dst_place.GetType() != AllocationType::CPU) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里为何要进行判断区分，直接通过dst_place拿到dev_ctx会有什么问题吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

之前windows有个test_yolo_loss的fluid测试，直接dst_place获取dev_ctx，会出错。
dst_place可能是CPU，src_place可能是CUDA，这会导致ctx_place是CPU。
最终会导致https://github.com/PaddlePaddle/Paddle/blob/701888a09cb6a89f136c92f96b85f440cad2e198/paddle/phi/core/tensor_utils.cc
120行throw异常
Context place error, excepted GPUPlace, but actually CPUPlace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YuanRisheng

Copy link
Contributor

Choose a reason for hiding this comment

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

修改超20个文件 PR-CI-APPROVAL 需要豁免
PR-CI-Coverage 覆盖率C++ 需要豁免
PR-CI-OP-benchmark 算子过多 一直超时，可能需要豁免

@engineer1109 op-benchmark是测试性能的，copy的修改可能会造成性能问题，多rerun几次看是否能跑过

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YuanRisheng 已经跑了一个星期了，又超时了。https://xly.bce.baidu.com/paddlepaddle/paddle/newipipe/detail/7756128/job/21674196

Copy link
Contributor

Choose a reason for hiding this comment

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

看起来没啥问题，下周我让相关同学再看一下

@engineer1109
Copy link
Contributor Author

Resolve conflict

@engineer1109
Copy link
Contributor Author

custom_device_test.cc有人在更新，改动放弃

YuanRisheng
YuanRisheng previously approved these changes Feb 10, 2023
View reviewed changes
zyfncg
zyfncg reviewed Feb 10, 2023
View reviewed changes
paddle/phi/common/scalar.cc Outdated
framework::TensorCopySync(tensor_in, cpu_place, &tensor);
phi::DeviceContextPool& pool = phi::DeviceContextPool::Instance();
auto dev_ctx = pool.Get(tensor_in.place());
phi::Copy(*dev_ctx, tensor_in, cpu_place, false, &tensor);
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.

@zyfncg scalar.cc已经纠正

@engineer1109
decouple tensor_utils
2e45dda 
fix X

remove TensorCopy

codestyle

add fluid memory header

fix symbol

fix cmake

fix cmake

fix context

fix header

fix place

fix context

fix context

fix context

fix code

fix custom context

fix custom context

fix copy

fix data_transform

fix style

remove changes of custom

fix scalar
@engineer1109
Copy link
Contributor Author

@zyfncg @YuanRisheng 还有什么问题吗？

ZzSean
ZzSean approved these changes Feb 13, 2023
View reviewed changes
Copy link
Contributor

@ZzSean ZzSean left a comment

Choose a reason for hiding this comment

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

LGTM for CI-OP-Benchmark

@engineer1109
Copy link
Contributor Author

@luotao1 这个PR现在如何？

@luotao1
Copy link
Contributor

luotao1 commented Feb 13, 2023

Coverage流水线已豁免

XiaoguangHu01
XiaoguangHu01 approved these changes Feb 14, 2023
View reviewed changes
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 057cdb9 into PaddlePaddle:develop Feb 14, 2023
@engineer1109
Copy link
Contributor Author

#47615 refer

@luotao1 luotao1 mentioned this pull request Feb 15, 2023
Closed
This was referenced Feb 22, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zyfncg zyfncg zyfncg approved these changes

@YuanRisheng YuanRisheng YuanRisheng approved these changes

@ZzSean ZzSean ZzSean approved these changes

@XiaoguangHu01 XiaoguangHu01 XiaoguangHu01 approved these changes

Assignees

@luotao1 luotao1

@YuanRisheng YuanRisheng

Labels
contributor External developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@engineer1109 @luotao1 @zyfncg @YuanRisheng @ZzSean @XiaoguangHu01