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 7th No.23】NO.23 为 Paddle 新增 ParameterDict API -part #68270

Merged
merged 10 commits into from
Oct 11, 2024

Conversation

Micalling
Copy link
Contributor

@Micalling Micalling commented Sep 17, 2024

PR Category

Inference

PR Types

Others

Description

【Hackathon 7th No.23】NO.23 为 Paddle 新增 ParameterDict API
RFC:PaddlePaddle/community#959
Docs:PaddlePaddle/docs#6874

Copy link

paddle-bot bot commented Sep 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.

@CLAassistant
Copy link

CLAassistant commented Sep 18, 2024

CLA assistant check
All committers have signed the CLA.

@Micalling
Copy link
Contributor Author

image
请问这是怎么回事?我是按照ParameterList写的

@Micalling
Copy link
Contributor Author

image 请问这是怎么回事?我是按照ParameterList写的

@luotao1

@Micalling
Copy link
Contributor Author

这个好像是需要你这里同意?
@jeff41404
image

@Micalling
Copy link
Contributor Author

Micalling commented Sep 21, 2024

image
还有这个似乎是在nn.__init__中添加了ParameterDict后需要确认
@luotao1 @jeff41404

Comment on lines 345 to 353
... tmp = self._helper.create_variable_for_type_inference('float32')
... self._helper.append_op(
... type="mul",
... inputs={"X": x,
... "Y": p},
... outputs={"Out": tmp},
... attrs={"x_num_col_dims": 1,
... "y_num_col_dims": 1})
... x = tmp
Copy link
Contributor

Choose a reason for hiding this comment

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

please use public APIs instead of non-public APIs(self._helper.create_variable_for_type_inference and self._helper.append_op) in the example code. for example, x = paddle.multiply(x, p) or x = x * p


def forward(self, x):
for i, (key, _) in enumerate(self.params):
x = _legacy_C_ops.mul(x, self.params[key])
Copy link
Contributor

Choose a reason for hiding this comment

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

it better to use public API to increase maintainability, e.g. paddle.matmul ?

@jeff41404
Copy link
Contributor

image 还有这个似乎是在nn.__init__中添加了ParameterDict后需要确认 @luotao1 @jeff41404

Yes, adding a public API requires confirmation. You don't need to handle this CI temporarily. Once you solve other problems and approved, PR-CI-Static-Check will pass

@jeff41404
Copy link
Contributor

please add link of rfc and docs in description above.

@Micalling
Copy link
Contributor Author

Micalling commented Sep 24, 2024

please add link of rfc and docs in description above.

已经将相应的链接加上去了,需要修改的也已经修改完毕。劳烦再review一下。另外也请review一下这个#68268 我觉得ci似乎有问题,我在 test/legacy_test/test_audio_functions.py 中的修改并没有执行,导致我一开始的单侧覆盖率一直过不去,后来我新加一个文件后,他却提示数据对不上,但我在本身的window电脑、aistudio的linux环境以及我朋友的mac电脑里面都是可以的,另外您也是可以看到,代码使用的都是paddle已经实现的api

zhangting2020
zhangting2020 previously approved these changes Sep 24, 2024
Copy link
Contributor

@zhangting2020 zhangting2020 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 PR-CI-Distribute-stable

jeff41404
jeff41404 previously approved these changes Sep 25, 2024
Copy link
Contributor

@jeff41404 jeff41404 left a comment

Choose a reason for hiding this comment

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

LGTM

@Micalling
Copy link
Contributor Author

你好,请问这个PI-CI-Static-Check需要怎么解决的? @sunzhongkai588

Copy link

paddle-ci-bot bot commented Oct 3, 2024

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

@monster1015
Copy link
Contributor

image
请问这是怎么回事? @luotao1

@Micalling
Copy link
Contributor Author

Micalling commented Oct 9, 2024

你好,请问能否review一下? @sunzhongkai588

@Micalling
Copy link
Contributor Author

image
image
请问这个是不是三个人都review的, @luotao1
http://preview-paddle-pr-68270.paddle-docs-preview.paddlepaddle.org.cn/documentation/docs/en/develop/api/paddle/nn/ParameterDict_en.html
然后这个文档构建好像也好了

sunzhongkai588
sunzhongkai588 previously approved these changes Oct 9, 2024
Copy link
Contributor

@sunzhongkai588 sunzhongkai588 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 review一下type annotation

self,
parameters: (
ParameterDict
| typing.Mapping[str, Tensor]
Copy link
Member

Choose a reason for hiding this comment

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

为什么要用 typing.Mapping 而不是已经 import 了的 collections.abc.Mapping

def __len__(self) -> int:
return len(self._parameters)

def __iter__(self) -> Iterator[tuple[str, Tensor]]:
Copy link
Member

Choose a reason for hiding this comment

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

为什么这个 __iter__ 返回结果的语义与 Python 的 dict 不一样,不是 dict 的 key 的 iterator,而是 key-value pair 的 iterator?这里的设计是基于什么?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

抱歉,这里是多加了的,参考layerdict然后看岔了

self,
parameters: (
ParameterDict
| typing.Mapping[str, Tensor]
Copy link
Member

Choose a reason for hiding this comment

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

同上

... super().__init__()
... # create ParameterDict with iterable Parameters
... self.params = paddle.nn.ParameterDict(
... {'t' + str(i): paddle.create_parameter(shape=[2, 2], dtype='float32') for i in range(num_stacked_param)})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
... {'t' + str(i): paddle.create_parameter(shape=[2, 2], dtype='float32') for i in range(num_stacked_param)})
... {f"t{i}": paddle.create_parameter(shape=[2, 2], dtype='float32') for i in range(num_stacked_param)})

Comment on lines 422 to 428
raise ValueError(
"The length of the "
+ str(i)
+ "'s element in parameters is "
+ str(len(kv))
+ ", which must be 2."
)
Copy link
Member

Choose a reason for hiding this comment

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

用 f-string

def __len__(self) -> int:
return len(self._parameters)

def __iter__(self) -> Iterator[tuple[str, Tensor]]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def __iter__(self) -> Iterator[tuple[str, Tensor]]:
def __iter__(self) -> Iterator[str]:
  1. 类型提示没改
  2. param_guard 不需要,因为返回的 iterator 访问不到参数

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的好的,已经修改了

@Micalling
Copy link
Contributor Author

image
请问这是怎么回事? @sunzhongkai588

@PaddlePaddle PaddlePaddle locked and limited conversation to collaborators Oct 10, 2024
@PaddlePaddle PaddlePaddle unlocked this conversation Oct 10, 2024
@SigureMo
Copy link
Member

CI 出了点问题,遇到类似问题可以考虑直接用一个空的 commit 重新触发 CI,或者 convert to draft + ready to review 来重新触发 CI

帮你用了另一种方式重新触发了全部 CI(lock + unlock,你没权限,但前两种你是有权限的)

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 🐾 for new API type annotations and print usage

@luotao1 luotao1 changed the title 【Hackathon 7th No.23】NO.23 为 Paddle 新增 ParameterDict API 【Hackathon 7th No.23】NO.23 为 Paddle 新增 ParameterDict API -part Oct 11, 2024
@luotao1 luotao1 merged commit e898b19 into PaddlePaddle:develop Oct 11, 2024
26 of 27 checks passed
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.

8 participants