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]Remove Profiler header (Part2) #50183

Merged
merged 10 commits into from
Feb 15, 2023
Merged

[PHI Decoupling]Remove Profiler header (Part2) #50183

merged 10 commits into from
Feb 15, 2023

Conversation

YuanRisheng
Copy link
Contributor

@YuanRisheng YuanRisheng commented Feb 3, 2023 •
edited
Loading

PR types

Others

PR changes

Others

Describe

迁移Profiler组件并移除对"paddle/fluid/platform/profiler/event_tracing.h"的依赖

@paddle-bot
Copy link

paddle-bot bot commented Feb 3, 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.

jiahy0825
jiahy0825 reviewed Feb 8, 2023
View reviewed changes
paddle/phi/core/os_info.h Outdated
@@ -0,0 +1,74 @@
/* Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2021->2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done,3q

paddle/phi/core/os_info.cc Outdated
@@ -0,0 +1,130 @@
/* Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2021->2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done,3q

paddle/phi/common/thread_data_registry.h Outdated
@@ -0,0 +1,175 @@
// Copyright (c) 2022 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2022->2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done,3q

paddle/phi/core/os_info.cc Outdated
#endif
#include "glog/logging.h"
#include "paddle/phi/common/thread_data_registry.h"
#include "paddle/phi/core/macros.h" // import DISABLE_COPY_AND_ASSIGN
Copy link
Contributor

Choose a reason for hiding this comment

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

delete useless comments

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/phi/core/os_info.h
@@ -0,0 +1,74 @@
/* Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.

Copy link
Contributor

Choose a reason for hiding this comment

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

为什么需要 os_info.h 和 os_info.cc 文件？对应 fluid 哪个文件中的实现？

Why do we need os_info.h and os_info.cc? Which fluid files are they corresponding to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

该文件是Profiler的依赖，从fluid下迁移过来的

zyfncg
zyfncg reviewed Feb 8, 2023
View reviewed changes
paddle/phi/api/profiler/CMakeLists.txt
@@ -0,0 +1,45 @@
proto_library(phi_profiler_proto SRCS profiler.proto)
Copy link
Contributor

Choose a reason for hiding this comment

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

这个profiler.proto会在phi中引入proto的依赖吗？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

paddle/phi/api/profiler/device_tracer.cc Outdated
@@ -22,11 +22,12 @@ limitations under the License. */
#include <thread> // NOLINT

#include "glog/logging.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.

这里引入了fluid的头文件，应该可以绕开

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/phi/core/os_info.cc Outdated
@@ -0,0 +1,130 @@
/* Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2021 -> 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

paddle/phi/core/os_info.h Outdated
@@ -0,0 +1,74 @@
/* Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
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/CMakeLists.txt Outdated
@@ -27,7 +27,7 @@ math_library(sequence_scale)
cc_library(
phi_data_layout_transform
SRCS data_layout_transform.cc
DEPS tensor blas)
DEPS tensor blas phi_profiler)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里是不是不需要依赖phi_profiler了？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done，3q

paddle/phi/tests/api/CMakeLists.txt Outdated
Comment on lines 5 to 16
DEPS phi_tensor glog phi_profiler)
elseif(WITH_ROCM)
hip_test(
test_phi_tensor
SRCS test_phi_tensor.cc
DEPS phi_tensor glog)
DEPS phi_tensor glog phi_profiler)
else()
cc_test(
test_phi_tensor
SRCS test_phi_tensor.cc
DEPS phi_tensor glog)
DEPS phi_tensor glog phi_profiler)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

这里需要依赖phi_profiler吗？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已删除

jiahy0825
jiahy0825 approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@jiahy0825 jiahy0825 left a comment

Choose a reason for hiding this comment

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

LGTM

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

@rainyfly rainyfly 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 platform event_tracing and related structure moving

XiaoguangHu01
XiaoguangHu01 approved these changes Feb 15, 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 8fabca1 into PaddlePaddle:develop Feb 15, 2023
@YuanRisheng YuanRisheng mentioned this pull request Mar 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzhang533 jzhang533 jzhang533 approved these changes

@zyfncg zyfncg zyfncg approved these changes

@jiahy0825 jiahy0825 jiahy0825 approved these changes

@rainyfly rainyfly rainyfly approved these changes

@XiaoguangHu01 XiaoguangHu01 XiaoguangHu01 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@YuanRisheng @jzhang533 @zyfncg @jiahy0825 @rainyfly @XiaoguangHu01