Skip to content

Conversation

@SigureMo
Copy link
Member

@SigureMo SigureMo commented Aug 31, 2025

PR Category

Execute Infrastructure

PR Types

Deprecations

Description

将获取所有 includes 提取成公共函数以免调整后需要修改 10 多处,resolves #74402 (comment)

另外 Python 3.2+ site 一定包含 getsitepackages 方法1,因此移除相关判断

Footnotes

  1. https://docs.python.org/3/library/site.html#site.getsitepackages

@SigureMo SigureMo requested a review from Copilot August 31, 2025 05:39
@paddle-bot
Copy link

paddle-bot bot commented Aug 31, 2025

你的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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extracts the logic for getting Paddle include directories into a common method to avoid code duplication across multiple files. The change introduces _get_all_paddle_includes_from_include_root() function and replaces repetitive include directory collection code in test files.

  • Introduces a new helper function _get_all_paddle_includes_from_include_root() in extension_utils.py
  • Refactors multiple test files to use the new common method instead of duplicated include path logic
  • Removes some unused imports and simplifies path construction in several places

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
python/paddle/utils/cpp_extension/extension_utils.py Adds the new helper function and refactors existing find_paddle_includes to use it
test/deprecated/custom_op/utils.py Replaces duplicated include path logic with the new helper function
test/custom_runtime/test_custom_op_setup.py Uses the new helper function for include path collection
test/custom_op/utils.py Replaces duplicated include path logic with the new helper function
test/custom_kernel/test_custom_kernel_load.py Simplifies site packages path collection
test/cpp_extension/utils.py Uses the new helper function for include path collection
test/cpp_extension/test_cpp_extension_jit.py Replaces duplicated include path logic with the new helper function
test/cpp_extension/cpp_extension_setup.py Uses the new helper function for include path collection
test/auto_parallel/semi_auto_parallel_simple_net_custom_relu.py Replaces duplicated include path logic with the new helper function
test/auto_parallel/semi_auto_parallel_for_custom_relu.py Uses the new helper function for include path collection
test/auto_parallel/custom_op/utils.py Replaces duplicated include path logic with the new helper function
python/paddle/base/core.py Simplifies site packages path collection

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +824 to +826
def _get_all_paddle_includes_from_include_root(include_root: str) -> list[str]:
"""
Return Paddle necessary include dir path.
Get all paddle include directories from include root (packaged in wheel)
Copy link

Copilot AI Aug 31, 2025

Choose a reason for hiding this comment

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

The function is missing a docstring that explains its parameters and return value. Consider adding documentation for the include_root parameter to clarify its expected format and purpose.

Copilot uses AI. Check for mistakes.
Comment on lines +24 to +25
from paddle.utils.cpp_extension.extension_utils import (
_get_all_paddle_includes_from_include_root,
Copy link

Copilot AI Aug 31, 2025

Choose a reason for hiding this comment

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

The function _get_all_paddle_includes_from_include_root is marked as private with a leading underscore but is being imported and used across multiple test files. Consider making it a public API by removing the underscore prefix or keeping it private and accessing it through a public interface.

Suggested change
from paddle.utils.cpp_extension.extension_utils import (
_get_all_paddle_includes_from_include_root,
get_all_paddle_includes_from_include_root,

Copilot uses AI. Check for mistakes.
@SigureMo SigureMo changed the title [CppExtension] Extract get paddle include dirs to a common method [CppExtension] Extract get paddle include dirs logic to a common method Aug 31, 2025
@SigureMo SigureMo requested review from gouzil and zrr1999 August 31, 2025 05:47
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (develop@40aeba6). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...thon/paddle/utils/cpp_extension/extension_utils.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop   #74994   +/-   ##
==========================================
  Coverage           ?   75.00%           
==========================================
  Files              ?        2           
  Lines              ?        8           
  Branches           ?        0           
==========================================
  Hits               ?        6           
  Misses             ?        2           
  Partials           ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SigureMo
Copy link
Member Author

SigureMo commented Sep 1, 2025

image image

这段代码绝对会被 CI 跑到,coverage rate 没过可能是自定义算子编译阶段不会计入覆盖率?

@SigureMo SigureMo merged commit 2f7ce55 into PaddlePaddle:develop Sep 1, 2025
77 of 79 checks passed
@SigureMo SigureMo deleted the cpp-ext/extract-get-paddle-include-dirs-to-common-method branch September 1, 2025 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants