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

fix test_var_base.py when FLAGS_enable_pir_api=True #62686

Merged
merged 10 commits into from
Mar 21, 2024

Conversation

zrr1999
Copy link
Member

@zrr1999 zrr1999 commented Mar 13, 2024

PR types

Others

PR changes

Others

Description

修复 FLAGS_enable_pir_api=True python3.11 test_var_base.py

Copy link

paddle-bot bot commented Mar 13, 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 Mar 13, 2024
Comment on lines 1568 to 1569
if not isinstance(dtype, paddle.dtype):
dtype = convert_np_dtype_to_dtype_(dtype)
Copy link
Member

Choose a reason for hiding this comment

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

对于 Variable 来说,应该只能是 VarType,但这里的 dtype 是不是可能是 DataType?

可以改成

if not isinstance(dtype, (DataType, VarType)):
    dtype = convert_np_dtype_to_dtype_(dtype)
if isinstance(dtype, DataType):
    dtype = paddle_type_to_proto_type[dtype]

这样下面 1612 行看起来也不可能是 DataType 了

唔,不过我们已经有很多类似的代码了,是否可以抽成一个函数?

Copy link
Member

Choose a reason for hiding this comment

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

根据后面的 comment,Variable 里是否不需要适配?

Copy link
Member Author

Choose a reason for hiding this comment

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

已修改

@@ -1103,9 +1104,18 @@ def _assert_to_static(self, var_base, static_var, is_param=False):
else:
self.assertTrue(isinstance(static_var, base.framework.Variable))

attr_keys = ['block', 'dtype', 'type', 'name']
attr_keys = ["block", "dtype", "type", "name"]
for attr in attr_keys:
Copy link
Member

Choose a reason for hiding this comment

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

原来 Variable 的 DataType 是从这里传进去的么?按理说 Variable 是不可能在 FLAGS_enable_pir_api 出现的

这里更合理的行为是使用 Value~可以看看好不好实现~

@@ -1239,7 +1249,7 @@ def test_print_tensor_dtype(self):
a = paddle.rand([1])
a_str = str(a.dtype)

expected = 'paddle.float32'
Copy link
Member

Choose a reason for hiding this comment

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

这里直接用 'paddle.float32' 是更合理的,虽然现在可能跑不通,但未来切换的时候肯定是要用 'paddle.float32' 的,所以这里先不改吧~

zrr1999 added 2 commits March 13, 2024 13:33
…hods.py, python/paddle/base/framework.py, and test/legacy_test/test_var_base.py
dtype = convert_np_dtype_to_dtype_(dtype)
if isinstance(dtype, core.DataType):
dtype = paddle_type_to_proto_type[dtype]
dtype = convert_np_dtype_to_proto_type(dtype)
Copy link
Member

Choose a reason for hiding this comment

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

这是不是有点问题?如果 dtype 已经是 DataType 的话,这个 DataType 就会传到后面了

if type(dtype) is str:
dtype = framework.convert_np_dtype_to_dtype_(dtype)

dtype = framework.convert_to_proto_type(dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 这里对于dtype的修改是不是应该放到if t.place.is_gpu_place():内,然后应该改为proto_dtype = framework.convert_to_proto_type(dtype)吧,因为dtype变量后面还会用到dtype != t_used.dtype,如果都变成proto dtype的话应该会有问题吧
  2. 另外需要改动下test/ir/pir/cinn/symbolic/test_llama_unsqueeze_expand.py的40h,把bool改成string

Copy link
Member Author

Choose a reason for hiding this comment

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

好的,已修改

@0x45f
Copy link
Contributor

0x45f commented Mar 18, 2024

下次不要在pr里大面积的把单引号改成双引号了,这样非常不好review :)

@zrr1999
Copy link
Member Author

zrr1999 commented Mar 18, 2024

下次不要在pr里大面积的把单引号改成双引号了,这样非常不好review :)

好的

@@ -37,7 +37,7 @@ def forward(self, x, y):
s2 = paddle.shape(y)[0]
s3 = paddle.shape(x)[1]

z = x.unsqueeze([1, 2]).cast(bool)
z = x.unsqueeze([1, 2]).cast(str)
Copy link
Member

@SigureMo SigureMo Mar 18, 2024

Choose a reason for hiding this comment

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

"bool" 😂,不是直接改成 str

Copy link
Member Author

Choose a reason for hiding this comment

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

好的,已修改

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 🐾

@0x45f 0x45f merged commit e6e7cff into PaddlePaddle:develop Mar 21, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants