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

【Hackathon 6th Fundable Projects 1 1-1】Add _typing module to paddle #63604

Merged
merged 21 commits into from
May 13, 2024

Conversation

Asthestarsfalll
Copy link
Contributor

@Asthestarsfalll Asthestarsfalll commented Apr 17, 2024

PR Category

Others

PR Types

New features

Description

大部分般自paddlepaddle-stubs

添加一些常用的以及可能会用到的,如Size1 Shape1D,名称可能还需要再考虑一下;后面还需要调研一下paddle内比较常用的类型提示有哪些

测试结果:
240424_19h56m26s_screenshot

Related links

Copy link

paddle-bot bot commented Apr 17, 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 Apr 17, 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.

注意一下与 paddlepaddle-stubs 不同的是,这里使用 .py 定义,所定义出来的是新的类型,是可能和运行时的类型混淆的(运行时真的有两个不同的 class),需要保证用户使用原有的类型时也是可以的,以及,用户一旦不小心使用了这里的 class 构造了运行时可访问的对象怎么办呢?

比如这里定义了 paddle._typing.dtype,如何保证它和 paddle.dtypepaddle.base.core.DataType)是一样的呢?最佳方式是为 paddle.base.core.DataType 提供 stub file,使其从根本上保持一致

Tensor 等 pybind 数据结构一样,都可以这样考虑

@megemini 觉得呢?

@megemini
Copy link
Contributor

注意一下与 paddlepaddle-stubs 不同的是,这里使用 .py 定义,所定义出来的是新的类型,是可能和运行时的类型混淆的(运行时真的有两个不同的 class),需要保证用户使用原有的类型时也是可以的,以及,用户一旦不小心使用了这里的 class 构造了运行时可访问的对象怎么办呢?

比如这里定义了 paddle._typing.dtype,如何保证它和 paddle.dtypepaddle.base.core.DataType)是一样的呢?最佳方式是为 paddle.base.core.DataType 提供 stub file,使其从根本上保持一致

Tensor 等 pybind 数据结构一样,都可以这样考虑

@megemini 觉得呢?

对!大原则参考 RFC 里面说的:

  • 非 Python 接口,提供 stub 标注文件
  • Python 接口,使用 inline 方式标注

可以这么想,_typing.xxx.py 里面不应该出现 的东西,比如 class dtype ... ,因为,已经有 paddle.dtype 了,如果目前就做静态类型检查,工具是可以检查出 paddle.dtype 的 ~

的东西应该在 pyi 这类 stub 文件里面,需要做的应该有两件事:

  • 一对一把接口搬过来,并且做类型标注,比如 Tensor.cast 这类方法
  • 补充 docstring,因为 pybind 的接口没法在静态检查的时候提示 docstring

我们无法直接标注 pybind 的接口,并且,目前的检查工具没法提示这类东西的 docstring,因此,需要我们补充 stub ~ 而 paddle.dtype 里面的 DataType 我觉得可以不做,因为,这个东西本身可以当作一个 ,而他的 docstring 没有啥具体作用 (print(paddle.dtype.FP16.__doc__) 没啥东西 ~)。相应的,不需要 _typing 里面增加 dtype 这个类,如果没有 docstring 的需要,那么 paddle.base.core.DataType 的 stub file 也不需要 ~

至于这个 _typing.dtype.py,感觉应该是 _typing._dtype_like.py ,可以参考 numpy 的 numpy/_typing/_dtype_like.py 。这里面只需要增加 Paddle 原来没有的东西,比如 _DTypeLiteral。另外,numpy 有个 numpy/numpy/_typing/_shape.py,这里就不是 _xxx_like.py ,而是 _xxx.py ,因为 numpy 原来没有 shape 这类的东西,因此需要补充,是全 的,可以体会一下 _xxx_like.py_xxx.py 的区别 ~

@SigureMo @Asthestarsfalll 帮忙看看这个思路有没有问题?

@megemini
Copy link
Contributor

@Asthestarsfalll 另外,这个任务需要补充 _typing 的测试用例和测试脚本哈 ~ 另起 PR 吗?

@SigureMo
Copy link
Member

如果没有 docstring 的需要,那么 paddle.base.core.DataType 的 stub file 也不需要 ~

这个我没有太理解,stub file 更主要提供的是类型信息,docstring 则是次要的,为什么没有 docstring 的需要就可以不用 stub file 了呢?

其实根据 typeshed 的规范1,stub file 不应该写 docstring 的,当然,这个在社区里也有争议,但这里以是否有 docstring 来评判是否加 stub file 我觉得是颠倒主次关系的

其余感觉没啥问题的~

Footnotes

  1. https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#conventions

@megemini
Copy link
Contributor

如果没有 docstring 的需要,那么 paddle.base.core.DataType 的 stub file 也不需要 ~

这个我没有太理解,stub file 更主要提供的是类型信息,docstring 则是次要的,为什么没有 docstring 的需要就可以不用 stub file 了呢?

我的表述有问题 ~~~ 是说这个 DataType 可能不需要 ~ 刚才试了,还是要加!

  • 添加 paddle/framework/dtype.pyi
    from enum import Enum
    
    class dtype(Enum):
        FP16 = ...
  • 添加 paddle/__init__.pyi
    from .framework.dtype import dtype
    __all__ = [
        "dtype",
    ]
  • 添加 py.typed

再使用 mypy 检查:

import paddle

a = paddle.dtype.FP16
reveal_type(a)

mypy 的输出:

note: Revealed type is "paddle.framework.dtype.dtype"

paddle.dtypepaddle.framework.dtype.dtype 是一个东西,应该没啥问题 ~

这里有两个问题:

  • class dtype(Enum): FP16 = ... 这样写行不?还需要把 FP16 也作为单独类?
  • 是不是可以在这个 PR 把 dtype.pyi 先加进去,paddle.__init__.pyi 放到后面任务一起加进去?不然,如果 paddle.__init__.pyi 不完整,会影响目前打包的提示功能 ~

其实根据 typeshed 的规范1,stub file 不应该写 docstring 的,当然,这个在社区里也有争议,但这里以是否有 docstring 来评判是否加 stub file 我觉得是颠倒主次关系的

其余感觉没啥问题的~

pybind 的接口如果用了 stub,好像必须得把 docstring 加进去,不然 vscode 我试了,貌似 docstring 的提示就没了?

@SigureMo

@SigureMo
Copy link
Member

pybind 的接口如果用了 stub,好像必须得把 docstring 加进去,不然 vscode 我试了,貌似 docstring 的提示就没了?

这是没问题的,因为 pybind API 的 docstring 是运行时才能获取的,VS Code(pylance)当然拿不到

我只是用来佐证「但这里以是否有 docstring 来评判是否加 stub file 我觉得是颠倒主次关系的」哈,没说我们不加,而且我们也会「优先」加 type annotation 而不是 docstring,这是这个任务 title 决定的

paddle.dtypepaddle.framework.dtype.dtype 是一个东西,应该没啥问题 ~

可以看看有没有可能让显示也是 paddle.dtype,会让用户看起来更友好些

开发时可以打开 inlay hints,调试会更方便些

image

class dtype(Enum): FP16 = ... 这样写行不?还需要把 FP16 也作为单独类?

应该没问题吧……可以试试直接枚举值就是 float16 这种么?FP16 是不暴露给用户侧的

@Asthestarsfalll
Copy link
Contributor Author

  • 补充 docstring,因为 pybind 的接口没法在静态检查的时候提示 docstring

我们无法直接标注 pybind 的接口,并且,目前的检查工具没法提示这类东西的 docstring,因此,需要我们补充 stub ~ 而 paddle.dtype 里面的 DataType 我觉得可以不做,因为,这个东西本身可以当作一个 ,而他的 docstring 没有啥具体作用 (print(paddle.dtype.FP16.__doc__) 没啥东西 ~)。

这里同意

相应的,不需要 _typing 里面增加 dtype 这个类,如果没有 docstring 的需要,那么 paddle.base.core.DataType 的 stub file 也不需要 ~

不加stub的话,我这里类型提示会为unknown

至于这个 _typing.dtype.py,感觉应该是 _typing._dtype_like.py ,可以参考 numpy 的 numpy/_typing/_dtype_like.py 。这里面只需要增加 Paddle 原来没有的东西,比如 _DTypeLiteral。另外,numpy 有个 numpy/numpy/_typing/_shape.py,这里就不是 _xxx_like.py ,而是 _xxx.py ,因为 numpy 原来没有 shape 这类的东西,因此需要补充,是全 的,可以体会一下 _xxx_like.py_xxx.py 的区别 ~

这里我明天看下numpy的类型

@Asthestarsfalll
Copy link
Contributor Author

@Asthestarsfalll 另外,这个任务需要补充 _typing 的测试用例和测试脚本哈 ~ 另起 PR 吗?

想先确定好具体实现,后续再添加

@Asthestarsfalll
Copy link
Contributor Author

paddle.dtypepaddle.framework.dtype.dtype 是一个东西,应该没啥问题 ~

可以看看有没有可能让显示也是 paddle.dtype,会让用户看起来更友好些

按照这种方法,我这里直接显示 dtype

240418_01h45m39s_screenshot

@Asthestarsfalll
Copy link
Contributor Author

240418_21h01m51s_screenshot
240418_21h04m02s_screenshot

python/paddle/framework/dtype.pyi Outdated Show resolved Hide resolved
python/paddle/_typing/shape.py Outdated Show resolved Hide resolved
python/paddle/_typing/shape.py Outdated Show resolved Hide resolved
python/paddle/_typing/shape.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
python/paddle/_typing/layout.py Outdated Show resolved Hide resolved
python/paddle/_typing/device_like.py Outdated Show resolved Hide resolved
python/paddle/_typing/basic.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
python/paddle/_typing/shape.py Outdated Show resolved Hide resolved

class Tensor: ...

def to_tensor(data, dtype=None, place=None, stop_gradient=True) -> Tensor: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

不在pyi里加的话,mypy会找不到,我本地的lsp也找不到,但是我搜索到的资料是优先从pyi中找,其次是py。但这样看起来直接不从py找了

Copy link
Contributor

Choose a reason for hiding this comment

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

是的 ~ 这个 init.pyi 文件现在暂时应该不需要 ~

当时 RFC 里面需要,是因为 Tensor 本来是想加在这里 ~

后面 @SigureMo 建议 Tensor 的 stub 文件 tensor.pyi 直接加在 tensor 目录里面,那么这个 init.pyi 应该暂时不需要了 ~

不然,mypy 优先从 pyi 查找,而这个 init.pyi 又不完整,就会导致 paddle.xxx 都找不到 ~

参考 #63953

Copy link
Contributor

Choose a reason for hiding this comment

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

不过,_typing 模块,貌似需要在 init.py 中引入

import paddle._typing as _typing

Copy link
Member

Choose a reason for hiding this comment

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

一般使用 from . import _typing as _typing

Copy link

paddle-ci-bot bot commented May 2, 2024

Sorry to inform you that dd9e04e's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

Copy link
Contributor

@megemini megemini left a comment

Choose a reason for hiding this comment

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

综合 review ,有几个问题:

具体涉及到 _typing 模块里面的东西,目前看应该够用,后面随着具体标注任务的进行,再修改也来得及 ~ 包括 _typing 模块的单测,可以放到后面再增加 ~

@Asthestarsfalll 辛苦!:)

@SigureMo 请帮忙看看上面的问题,尤其是 setup 和 test_type_hints 的配置,我也吃不准 ~

感谢二位!!!


class Tensor: ...

def to_tensor(data, dtype=None, place=None, stop_gradient=True) -> Tensor: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

是的 ~ 这个 init.pyi 文件现在暂时应该不需要 ~

当时 RFC 里面需要,是因为 Tensor 本来是想加在这里 ~

后面 @SigureMo 建议 Tensor 的 stub 文件 tensor.pyi 直接加在 tensor 目录里面,那么这个 init.pyi 应该暂时不需要了 ~

不然,mypy 优先从 pyi 查找,而这个 init.pyi 又不完整,就会导致 paddle.xxx 都找不到 ~

参考 #63953


class Tensor: ...

def to_tensor(data, dtype=None, place=None, stop_gradient=True) -> Tensor: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

不过,_typing 模块,貌似需要在 init.py 中引入

import paddle._typing as _typing

Comment on lines 29 to 35
class _Place(Protocol):
def __init__(self, id: int) -> None:
...


NPUPlace = _Place
MLUPlace = _Place
Copy link
Contributor

@megemini megemini May 9, 2024

Choose a reason for hiding this comment

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

xxx = _xxx 这样写有点问题,比如:

from __future__ import annotations

class _Place:
    def __init__(self, id: int) -> None:
        ...

NPUPlace = _Place
MLUPlace = _Place

def test(place: NPUPlace) -> None:
    return

a = MLUPlace(0)
test(a)

这里面 a 类型实际是错的,但是 mypy 检查 pass ~

直接使用 = 会使两个类型没啥区别 ~

如果要写的话,还是单独列一个类 ~

不过,建议 NPUPlace MLUPlace 暂时不要了,因为 paddle 目前还没有暴露出来这俩个东西,只在 pyi 里面写可能会有异议 ~ 尽量不要增加 _typing 之外属于 api 的东西吧 ~

PlaceLike 支持 str 应该目前用足够 ~ 这样的话,init.py 和 init.pyi 也不需要这两个东西 ~

@SigureMo 看看行不?

Copy link
Member

@SigureMo SigureMo May 9, 2024

Choose a reason for hiding this comment

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

protocol 应该继承,(或者说是实现,这是组合的概念),而不是直接 assign,NPU 和 MLU 很早之前就在框架内退场了吧大概一年前左右的开源任务

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
Member

Choose a reason for hiding this comment

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

对的,没有就删掉好了

Comment on lines +22 to +23
Tuple[Union[int, Tensor, None], ...],
List[Union[int, Tensor, None]],
Copy link
Contributor

Choose a reason for hiding this comment

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

使用 tuplelist 吧 ~

另外,需要 from __future__ import annotations

这里手误写错了,不过我在本地测试过,这些代码都没问题,3.8.0可以使用list作为类型提示,现在添加了mypy进行测试

python 3.8 应该不能直接用 tuplelist,我这里是 3.8.17

----> 1 a:list[int] = [1,2]

TypeError: 'type' object is not subscriptable

In [2]: from __future__ import annotations

In [3]: a:list[int] = [1,2]

你那边测试可能是 mypy 指定版本有问题?

@@ -19,3 +19,4 @@ wandb>=0.13 ; python_version<"3.12"
xlsxwriter==3.0.9
xdoctest==1.1.1
ubelt==1.3.3 # just for xdoctest
mypy
Copy link
Contributor

Choose a reason for hiding this comment

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

统一一下版本号吧 mypy==1.10.0


class TestTypeHints(unittest.TestCase):
def check_with_mypy(self, file_path):
stdout, stderr, exitcode = api.run([file_path])
Copy link
Contributor

Choose a reason for hiding this comment

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

搞一个配置文件吧 ~

https://github.com/PaddlePaddle/Paddle/pull/63901/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711

后面统一使用一个配置文件,如果你这里加了,后面我那里可以去掉~

@SigureMo
Copy link
Member

SigureMo commented May 9, 2024

test_type_hints.py CI 中没有测试到,应该也需要配置一下 ~

需要配置 CMakeLists.txt 才可以跑,不过我之前说的是确保「运行时」没问题,其实按照上面说的加一个 from . import _typing as _typing 就能测到了,不需要加这些「静态检查」单测,按照之前 RFC 讨论,我们使用示例代码保证静态检查,不再额外设置类型提示单测

因此本 PR 就不涉及 mypy 的添加什么的吧,相关内容不要做重了,mypy 在 #63901 考虑

尤其是 setup

setup.py 我没什么使用经验,我大多使用的是基于 PEP 517 pyproject.toml 的现代化打包方式,这个的话只有测试验证才知道是否可行,如果打包后发现 wheel 包里确实有这些文件,就没问题

@Asthestarsfalll
Copy link
Contributor Author

@megemini @SigureMo
更新了一下:

  • 先移除了测试相关代码,如有需要后续添加
  • 添加了 from . import _typing as _typing
  • 添加了 framework/init.pyi
  • 在setup.py里添加了相关打包内容,但是我编译后安装并没有打包成功

@megemini
Copy link
Contributor

  • 添加了 framework/init.pyi

可以不用添加这个吧? paddle.__init__.py 里面已经有了 ~

  • 在setup.py里添加了相关打包内容,但是我编译后安装并没有打包成功

有两个地方要改,

  • python/setup.py.in
  • setup.py

#63901

@SigureMo
Copy link
Member

我来改一下这个 PR,尽快推动本 PR 合入,以免阻塞整个项目进度

@megemini
Copy link
Contributor

megemini commented May 10, 2024

我来改一下这个 PR,尽快推动本 PR 合入,以免阻塞整个项目进度

还有 #63953 没 review,是不是漏掉了 😂

@SigureMo
Copy link
Member

还有 #63953 没 review,是不是漏掉了 😂

没有哈,现在注意力在这俩上

@SigureMo SigureMo changed the title 【Hackathon 6th Fundable Projects 1 1-1】Add _typing module to paddle 【Hackathon 6th Fundable Projects 1 1-1】Add _typing module to paddle May 10, 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 merged commit 8b19229 into PaddlePaddle:develop May 13, 2024
30 of 31 checks passed
co63oc pushed a commit to co63oc/Paddle that referenced this pull request May 13, 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.

6 participants