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

[Typing] Ensure packaging tensor.pyi into wheel #64575

Merged
merged 24 commits into from
Jun 4, 2024

Conversation

megemini
Copy link
Contributor

@megemini megemini commented May 24, 2024

PR Category

User Experience

PR Types

Bug fixes

Description

  • 修改 CMakeLists.txt,在 pack 前生成 tensor.pyi
  • 复制 tensor.pyi 到源码目录中

@SigureMo 请评审 ~

Related links

Copy link

paddle-bot bot commented May 24, 2024

你的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 the contributor External developers label May 24, 2024
COMMAND
${CMAKE_COMMAND} -E copy
${PADDLE_BINARY_DIR}/python/paddle/tensor/tensor.pyi
${PADDLE_SOURCE_DIR}/python/paddle/tensor/tensor.pyi
Copy link
Member

@SigureMo SigureMo May 24, 2024

Choose a reason for hiding this comment

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

话说这几部分有可以复用的么?是否可以写成函数?

from paddle import _C_ops

try:
from paddle import version
Copy link
Member

Choose a reason for hiding this comment

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

这里同样,删掉 version import,改成 paddle.version 就好了

@megemini
Copy link
Contributor Author

Update 20240525

  • tools/gen_tensor_stub.py 中增加 add_dll_directory 函数,用于在 win 系统下避免 dll 加载失败。

    原因:paddle 在编译时,会生成或拷贝(第三方)库文件,类 unix 是 so, win 是 dll
    类 unix 系统在编译时,会把这些库加到路径里面,但是 win 系统只是生成了库文件,但是路径里面没有。
    libpaddle.pyd 虽然存在,但是其依赖的 dll 库(libs目录中)却是空的,而系统路径里面又没有,从而导致之前 win 系统下 import paddle 时提示 from . import libpaddle 出现 dll 加载错误。
    之前的 PR 【Type Hints】Paddle 中引入 Tensor stub 文件 #63953 之所以没问题,是因为,paddle 在打包 wheel 的时候,setup.py 会拷贝库文件统一到 libs 目录里面的,因此,打包之后再 import 就不会出错。

    因此,解决方案有两个:

    • CMakeLists 中把 win 依赖的库目录加到系统中去
      优点:下游的脚本,如 tools/gen_tensor_stub.py 不需要再手动添加
      缺点:复杂,容易出错。另外,没有本地测试环境,调试麻烦。
    • tools/gen_tensor_stub.py 在 import paddle 前手动 add_dll_directory ,把这些库目录加到路径中
      优点:简单
      缺点:如果上游,CMakeLists 和 setup.py 等修改了,仍需要改此文件

    基于此,此处选择第二种方案。

  • 修改 CMakeLists.txt

    • 添加 paddle_stub 编译 target
    • 添加 DEPENDS ${PADDLE_PYTHON_BUILD_DIR}/.timestamp ,还原 PR fix mac m1 bug #63158
  • 修改 paddle.__init__.py ,添加标记 is_paddle_installed

  • 修改若干文件对于 paddle.version 的引入

@SigureMo 请评审 ~

Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

其余感觉没啥问题,等 6.2 拉分支之后会合入看看效果,如果 QA 那边没有反馈更多非 CI 流水线监控硬件编译问题,就没啥问题了~

Comment on lines 458 to 462
def generate_stub_file(input_file=None, output_file=None):
if input_file is None or output_file is None:
args = parse_args()
input_file = args.input_file
output_file = args.output_file
Copy link
Member

Choose a reason for hiding this comment

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

可以考虑改成

def generate_stub_file(input_file, output_file): ...
def main():
    args = parse_args()
    generate_stub_file(args.input_file, args.output_file)

这样分层应该会更合理些

Comment on lines 330 to 339
try:
import paddle
except ImportError:
sys.stderr.write(
'''ERROR: Can NOT import paddle.
We could import paddle without installation, with all libs (.dll or .so) copied into dir `paddle/libs`,
or path already been set for the system.
'''
)
raise
Copy link
Member

Choose a reason for hiding this comment

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

可以考虑抽成函数:

def try_import_paddle() -> types.ModuleType | None: ...
def get_tensor_members():
    paddle = try_import_paddle()
    if not paddle:
        raise Xxx()
    ...

@@ -14,12 +14,16 @@

import typing

is_paddle_installed = False
Copy link
Member

Choose a reason for hiding this comment

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

这里从其他开发者角度来看会有点奇怪,为什么没有安装却可以调用呢?这里是否可以赋予更细致的语义?比如这里应该是信息已生成,是否可以用 __is_metainfo_generated?加 __ 以免误 import,毕竟这是 __init__

@megemini
Copy link
Contributor Author

Update 20240528

  • setup.py.in 中添加 check_build_dependency 方法,对齐 setup.py

这里只有一处不同,原 setup.py

def check_build_dependency():
    missing_modules = '''Missing build dependency: {dependency}
Please run 'pip install -r python/requirements.txt' to make sure you have all the dependencies installed.
'''.strip()

    with open(TOP_DIR + '/python/requirements.txt') as f:

取的目录是 TOP_DIRTOP_DIR = os.path.dirname(os.path.realpath(__file__))

而由于 setup.py.insetup.py 不是一个文件,特使用 ${PADDLE_SOURCE_DIR} 代替:

def check_build_dependency():
    missing_modules = '''Missing build dependency: {dependency}
Please run 'pip install -r python/requirements.txt' to make sure you have all the dependencies installed.
'''.strip()

    with open('${PADDLE_SOURCE_DIR}' + '/python/requirements.txt') as f:

这两个目录应该是一样的,如有特殊情况还望告知 ~

@SigureMo

@SigureMo SigureMo changed the title 【Type Hints】CMakeLists.txt for pack tensor.pyi [Typing] Ensure pack tensor.pyi into wheel Jun 2, 2024
Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTMeow 🐾

@SigureMo SigureMo changed the title [Typing] Ensure pack tensor.pyi into wheel [Typing] Ensure packaging tensor.pyi into wheel Jun 4, 2024
Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

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

LGTM

@SigureMo SigureMo merged commit 09ce1b8 into PaddlePaddle:develop Jun 4, 2024
32 checks passed
SigureMo added a commit that referenced this pull request Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants