[Typing][C-40][BUAA] Add type annotations for 1 file in `python/paddle/distributed/fleet/base/distributed_strategy.py` #67405

Whsjrczr · 2024-08-14T02:25:34Z

PR Category

User Experience

PR Types

Improvements

Description

类型标注：

python/paddle/distributed/fleet/base/distributed_strategy.py

Related links

[Type Hints] 为公开 API 标注类型提示信息 #65008

@SigureMo @megemini

… develop

paddle-bot · 2024-08-14T02:25:39Z

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

… th001

Whsjrczr · 2024-08-15T04:27:19Z

请求review

megemini

没有全部 review 完，问题比较多，主要集中在 TypedDict 的使用上，可以看一下怎么用这个类型标注～另外，即使是 dict ，也需要两个参数，如 dict[x,y] 而不是 dict[x] 。

还有，需要标注 DistributedStrategy 中的实例属性，如 strategy，参考 #67448 (review)

最后，PR 的描述不规范，看一下 https://github.com/PaddlePaddle/Paddle/issues/65008 是如何要求的

megemini · 2024-08-15T10:03:41Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -299,7 +307,7 @@ def build_strategy(self):

    @build_strategy.setter
    @is_strict_auto
-    def build_strategy(self, strategy):
+    def build_strategy(self, strategy: str) -> None:


Suggested change

def build_strategy(self, strategy: str) -> None:

def build_strategy(self, strategy: BuildStrategy) -> None:

megemini · 2024-08-15T10:05:05Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -313,7 +321,7 @@ def build_strategy(self, strategy):
                )

    @property
-    def gradient_scale_configs(self):
+    def gradient_scale_configs(self) -> dict[str]:


Suggested change

def gradient_scale_configs(self) -> dict[str]:

def gradient_scale_configs(self) -> dict[str, Any]:

megemini · 2024-08-15T10:07:08Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -332,7 +340,7 @@ def gradient_scale_configs(self):

    @gradient_scale_configs.setter
    @is_strict_auto
-    def gradient_scale_configs(self, config):
+    def gradient_scale_configs(self, config: dict[str]) -> None:


Suggested change

def gradient_scale_configs(self, config: dict[str]) -> None:

def gradient_scale_configs(self, config: dict[str, Any]) -> None:

megemini · 2024-08-15T10:08:19Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -367,7 +375,7 @@ def a_sync(self):

    @a_sync.setter
    @is_strict_auto
-    def a_sync(self, flag):
+    def a_sync(self, flag: bool) -> None:


def a_sync(self): 没标注

megemini · 2024-08-15T10:09:18Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -377,7 +385,7 @@ def a_sync(self, flag):
            )

    @property
-    def a_sync_configs(self):
+    def a_sync_configs(self) -> dict[int | bool]:


使用 TypedDict

megemini · 2024-08-15T10:18:14Z

python/paddle/distributed/fleet/base/distributed_strategy.py

        assert isinstance(flag, bool), "qat should have value of bool type"
        self.strategy.qat = flag

    @property
-    def qat_configs(self):
+    def qat_configs(self) -> dict[bool | int | str | list[str]]:


TypedDict

megemini · 2024-08-15T10:18:23Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -1085,12 +1103,12 @@ def qat_configs(self):
        return get_msg_dict(self.strategy.qat_configs)

    @qat_configs.setter
-    def qat_configs(self, configs):
+    def qat_configs(self, configs: dict[bool | int | str | list[str]]) -> None:


megemini · 2024-08-15T10:19:34Z

python/paddle/distributed/fleet/base/distributed_strategy.py

        if isinstance(flag, bool):
            self.strategy.recompute = flag
        else:
            logger.warning("recompute should have value of bool type")

    @property
-    def sync_nccl_allreduce(self):
+    def sync_nccl_allreduce(self) -> None:


Suggested change

def sync_nccl_allreduce(self) -> None:

def sync_nccl_allreduce(self) -> bool:

megemini · 2024-08-15T10:26:21Z

python/paddle/distributed/fleet/base/distributed_strategy.py

        if isinstance(value, int):
            self.strategy.fuse_grad_size_in_MB = value
        else:
            logger.warning("fuse_grad_size_in_MB should have value of int type")

    @property
-    def last_comm_group_size_MB(self):
+    def last_comm_group_size_MB(self) -> float:


Suggested change

def last_comm_group_size_MB(self) -> float:

def last_comm_group_size_MB(self) -> int:

megemini · 2024-08-15T10:26:37Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -1295,14 +1313,14 @@ def last_comm_group_size_MB(self):

    @last_comm_group_size_MB.setter
    @is_strict_auto
-    def last_comm_group_size_MB(self, value):
+    def last_comm_group_size_MB(self, value: float) -> None:


Suggested change

def last_comm_group_size_MB(self, value: float) -> None:

def last_comm_group_size_MB(self, value: int) -> None:

请问TypedDict是需要用class定义吗

https://docs.python.org/3.14/library/typing.html#typing.TypedDict

megemini · 2024-08-15T10:42:07Z

关联 #65008

megemini

这里涉及较多 protobuf 与私有的一些成员，比较麻烦，辛苦～

p.s. 另外，函数内部的函数，一般不需要标注。

megemini · 2024-08-22T03:55:02Z

python/paddle/distributed/fleet/base/distributed_strategy.py

        check_configs_key(
            self.strategy.fs_client_param, configs, "fs_client_param"
        )
        assign_configs_value(self.strategy.fs_client_param, configs)

    @property
-    def sparse_table_configs(self):
+    def sparse_table_configs(self) -> SparseTableConf:


SparseTableConf 的地方改为 dict[str, Any] 吧～

首先，根据代码中的逻辑，

for table_name in configs: ...

configs 应该是 dict[str, dict[str, Any]] 的形式，是个嵌套的 dict ；其次，嵌套的 dict 不只包括列表中的东西，如 if not configs.get("use_cvm", True): ～而且，这个表太大了，后面维护会很困难，所以建议用 dict[str, Any] 替代～

megemini · 2024-08-22T03:56:50Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -772,7 +969,9 @@ def sparse_optimizer_config(sgd, strategy, prefix):
                )
                sgd.adam.weight_bounds.extend(bounds)

-        def set_sparse_table_config(table_data, config):
+        def set_sparse_table_config(
+            table_data: Table, config: dict[float]


几个问题：

dict 需要两个参数

这里不是 Table

内部函数一般不需要标注

这里涉及到 distributed 模块内部的一些 protobuf 成员，不标注吧～

megemini · 2024-08-22T04:40:38Z

python/paddle/distributed/fleet/base/distributed_strategy.py

+        def sparse_optimizer_config(
+            sgd: Table, strategy: DistributedStrategy, prefix: str
+        ) -> None:


megemini · 2024-08-22T04:50:16Z

python/paddle/distributed/fleet/base/distributed_strategy.py

@@ -189,38 +368,50 @@ def __init__(self):
        DistributedStrategy supports configurations from BuildStrategy.

        """
-        self.strategy = distributed_strategy_pb2.DistributedStrategy()
+        self.strategy: DistributedStrategy = (


Suggested change

self.strategy: DistributedStrategy = (

self.strategy: Any = (

这里的 strategy 与 DistributedStrategy 不是同一个东西，distributed_strategy_pb2.DistributedStrategy 是 protobuf 生成的，这里直接用 Any 表示吧～

megemini · 2024-08-22T05:08:33Z

python/paddle/distributed/fleet/base/distributed_strategy.py

+    from paddle.static import BuildStrategy
+
+
+class SyncConf(TypedDict):


这一部分 TypedDict 的定义：

放到 TYPE_CHECKING 内部

使用 _ 开头，表示内部使用，如 _SyncConf

统一改为 total=False 的形式，如 class _SyncConf(TypedDict, total=False)

统一不加 | None，除非文档中明确表示可以用 None

不添加 SparseTableConf

megemini

LGTM ~

Whsjrczr added 7 commits August 12, 2024 15:35

2 api

0c9b893

update broadcast tensor

0ec2002

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

749866d

… develop

typehint in 1 file

4fd2485

update if

9e04062

undo changes

ee4bb32

type hint in 1 file

48603ff

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

745ec1b

… th001

luotao1 added contributor External developers HappyOpenSource Pro 进阶版快乐开源活动，更具挑战性的任务 labels Aug 14, 2024

luotao1 assigned luotao1 and SigureMo Aug 14, 2024

megemini mentioned this pull request Aug 14, 2024

[Type Hints] 为公开 API 标注类型提示信息 #65008

Closed

megemini reviewed Aug 15, 2024

View reviewed changes

Whsjrczr added 3 commits August 16, 2024 15:40

update

a8e95d8

Merge branch 'th001' of https://github.com/Whsjrczr/Paddle into th001

a03fb9c

update TypedDict

3a95c33

megemini reviewed Aug 22, 2024

View reviewed changes

update

a6aaf43

megemini approved these changes Aug 23, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

Whsjrczr mentioned this pull request Aug 23, 2024

[WeeklyReport]Whsjrczr 2024.08.10 ~ 2024.08.23 PFCCLab/Starter#357

Merged

luotao1 approved these changes Aug 23, 2024

View reviewed changes

luotao1 merged commit e77a4b0 into PaddlePaddle:develop Aug 23, 2024
28 of 29 checks passed

Whsjrczr mentioned this pull request Aug 26, 2024

[WeeklyReports] 2024.08.10~2024.08.23 周报收集 PFCCLab/Starter#340

Closed

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Typing][C-40][BUAA] Add type annotations for 1 file in `python/paddle/distributed/fleet/base/distributed_strategy.py` #67405

[Typing][C-40][BUAA] Add type annotations for 1 file in `python/paddle/distributed/fleet/base/distributed_strategy.py` #67405

Whsjrczr commented Aug 14, 2024 •

edited

Loading

paddle-bot bot commented Aug 14, 2024

Whsjrczr commented Aug 15, 2024

megemini left a comment

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

megemini Aug 15, 2024

Whsjrczr Aug 20, 2024

megemini Aug 20, 2024

megemini commented Aug 15, 2024

megemini left a comment •

edited

Loading

megemini Aug 22, 2024

megemini Aug 22, 2024

megemini Aug 22, 2024

megemini Aug 22, 2024

megemini Aug 22, 2024

megemini left a comment

This comment was marked as duplicate.

	def build_strategy(self, strategy: str) -> None:
	def build_strategy(self, strategy: BuildStrategy) -> None:

	def gradient_scale_configs(self) -> dict[str]:
	def gradient_scale_configs(self) -> dict[str, Any]:

	def gradient_scale_configs(self, config: dict[str]) -> None:
	def gradient_scale_configs(self, config: dict[str, Any]) -> None:

	def sync_nccl_allreduce(self) -> None:
	def sync_nccl_allreduce(self) -> bool:

	def last_comm_group_size_MB(self) -> float:
	def last_comm_group_size_MB(self) -> int:

	def last_comm_group_size_MB(self, value: float) -> None:
	def last_comm_group_size_MB(self, value: int) -> None:

	self.strategy: DistributedStrategy = (
	self.strategy: Any = (

		from paddle.static import BuildStrategy


		class SyncConf(TypedDict):

[Typing][C-40][BUAA] Add type annotations for 1 file in python/paddle/distributed/fleet/base/distributed_strategy.py #67405

[Typing][C-40][BUAA] Add type annotations for 1 file in python/paddle/distributed/fleet/base/distributed_strategy.py #67405

Conversation

Whsjrczr commented Aug 14, 2024 • edited Loading

PR Category

PR Types

Description

Related links

paddle-bot bot commented Aug 14, 2024

Whsjrczr commented Aug 15, 2024

megemini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megemini commented Aug 15, 2024

megemini left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megemini left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

[Typing][C-40][BUAA] Add type annotations for 1 file in `python/paddle/distributed/fleet/base/distributed_strategy.py` #67405

[Typing][C-40][BUAA] Add type annotations for 1 file in `python/paddle/distributed/fleet/base/distributed_strategy.py` #67405

Whsjrczr commented Aug 14, 2024 •

edited

Loading

megemini left a comment •

edited

Loading