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

[Dy2St] pir dy2st unittest verification - Part 9 #59232

Merged
merged 11 commits into from
Nov 29, 2023

Conversation

gouzil
Copy link
Member

@gouzil gouzil commented Nov 21, 2023

PR types

Others

PR changes

Others

Description

状态 单测 错误类别 备注 报错信息
test_train_step 整图导出 train_step 整图导出暂不支持 PIR AttributeError: 'paddle.base.libpaddle.pir.OpResult' object has no attribute 'backward'
test_train_step_resnet18_sgd 整图导出 同 train_step
test_train_step_resnet18_adam 整图导出 同 train_step
🚧 test_tensor_shape: {TestOpNumWithTensorShapeInIf1, TestOpNumWithTensorShapeInFor1, TestOpNumWithTensorShapeInWhile1} 控制流 三个 case 里有控制流
test_mnist: test_mnist_to_static 动转静执行 段错误,已经定位到在 partial_program_layer _prepare_attributes 时候报错,self.program 里调用 _append_backward_desc 会报错
test_tensor_methods size的改动可以看一下#58987
test_tensor_memcpy_on_gpu API 依赖#59300 这我适配的,就不贴报错了
test_for_enumerate.py: TestForIterVarNumpy API #59115 已修,注意只是这个 case 好了,不是整个单测好了
test_for_enumerate.py: TestForEnumerateVarWithNestedRange 控制流 上面的大多数修好了,但是继承自 TestForIterVarNumpyTestForEnumerateVarWithNestedRange 包含控制流,所以不支持;另外注意这个 case 中间态也有问题,中间态会在 Win 和 mac CI 上挂掉,本地没问题

相关链接:

@gouzil gouzil requested a review from SigureMo November 21, 2023 16:55
@paddle-bot paddle-bot bot added the contributor External developers label Nov 22, 2023
SigureMo
SigureMo previously approved these changes Nov 23, 2023
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

@@ -39,8 +39,10 @@


def convert_attr(x, attr):
Copy link
Member

Choose a reason for hiding this comment

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

elif 好像没必要存在?直接删掉即可

这里应该是为了对齐动态图 x.size 和老 IR x.size() 动静不统一的问题的,在 PIR 下动静统一,所以 convert_attr 以及 AttributeJstTransformer 应该都是可以清理了的

我觉得我们可以记一个 TODO(cleanup-legacy-ir),说明下老 IR 退场时是可以直接删掉相关 convert 和 Transformer

Copy link
Member

Choose a reason for hiding this comment

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

已直接修改

@@ -356,6 +372,43 @@ def clone(self):
"""
return paddle.assign(self)

@fake_interface_only
def clear_gradient(self):
Copy link
Member

Choose a reason for hiding this comment

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

TODO: 等 CI 过了让震哥来 review API 变动

@SigureMo
Copy link
Member

需要解决下冲突

…_push_9

# Conflicts:
#	python/paddle/pir/math_op_patch.py
@gouzil
Copy link
Member Author

gouzil commented Nov 24, 2023

需要解决下冲突

done

gouzil and others added 2 commits November 27, 2023 21:27
…_push_9

# Conflicts:
#	test/dygraph_to_static/test_mnist.py
#	test/dygraph_to_static/test_tensor_methods.py
#	test/dygraph_to_static/test_tensor_shape.py
#	test/dygraph_to_static/test_train_step.py
@@ -31,6 +32,21 @@
]


def _fake_interface_only_(func):
Copy link
Contributor

Choose a reason for hiding this comment

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

framework.py中有一个相同的_fake_interface_only_函数,能直接用framework.py中的那个函数吗?

Copy link
Member Author

Choose a reason for hiding this comment

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

会循环引用emmm,得解了才能复用

@0x45f 0x45f self-assigned this Nov 28, 2023
Copy link
Contributor

@0x45f 0x45f 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 math_op_patch.py

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.

Great work!

to_static,
prediction,
[img.name],
img = to_variable(dy_x_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

后续遇到类似to_variable这种旧的API,可否替换为paddle.to_tensor?

Copy link
Member

Choose a reason for hiding this comment

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

可以的,已经有清理一些了,这应该是没注意到的

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 for docs (反正base的文档不暴露嘻嘻

@SigureMo
Copy link
Member

LGTM for docs (反正base的文档不暴露嘻嘻

image

@SigureMo SigureMo merged commit cb59229 into PaddlePaddle:develop Nov 29, 2023
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.

6 participants