-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[Pir]Fix Value eq error when using set #58896
Conversation
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
tmp_other = other.value if isinstance(other, ValueWrapper) else other | ||
if self.value is None: | ||
return tmp_other is None | ||
return self.value.is_same(tmp_other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里如果tmp_other也不是一个pir::Value呢,这里是不是就直接报错了?但其实应该要返回False的,因为对象类似不匹配
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
而且从实现上来看,这里self.value 也不一定是pir::Value? 若是的话,它就不一定有is_same 方法
… fix-opresult-eq
…e/Paddle into wangzhen-opresult-map
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
… fix-opresult-eq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for ir_backward
PR types
Bug fixes
PR changes
Others
Description
Pcard-67164
1 背景
我们需要在python端给pir下的OpResult patch一些方法,以对齐老静态图的Variable,比如x.reshape、x.add等方法的调用,以及a == b,a != b等操作符也需要对齐。
但是在给OpResult patch __eq__方法的时候遇到了问题,相关PR:https://github.com/PaddlePaddle/Paddle/pull/5834 。在静态图下对于a == b 这样的操作(也就是OpResult的__eq__方法)的语义是在program中插入一个 equal op,这是__eq__的正确语义,请牢记。
2 具体问题
在python端给OpResult patch __eq__方法的时候(实现eq的正确语义,也就是插入一个equal op),发现pir backward的逻辑会有问题,发现之前在C++端的pir.cc中给OpResult和Value已经pybind了__eq__的方法,逻辑如下是通过impl来判断OpResult或者Value是否是同一个对象的。后面发现不止backward,pir optest、组合算子逻辑、pir单测等多个地方都有类似的用法
之前为什么会加这样的逻辑呢?
因为ir_backward.py中有类似set(Value)、set(OpResult)的用法,对于set来说需要去重那set是如何去重的呢?是通过item的__hash__,对象指针和__eq__来联合判断两个元素是否是同一个元素,所以为了能够正确的使用set,就在C++端pybind 了 __hash__和__eq__方法。
但是这里的__eq__的语义和上面提到的正确语义是相背的(正确语义是插入equal op)。
在PIR下我们主要的诉求就是:给Value patch上__eq__方法,该方法应该在program中插入equal op,同时已有的set、dict等代码能够正确work
3 Python集合说明
3.1 set和dict
set.add判断是否是同一个元素的步骤如下:
3.2 list
对于
if x in list
以及list.index(x)
这样的python代码,会调用PyObject_RichCompareBool。PyObject_RichCompareBool内Py_EQ的时候4 老静态图类似情况说明
老静态图下因为Variable有name,所以很多地方使用的都是类似set(name)这种形式,set(name)这种用法是没有问题的。
对于set(Variable),老静态图也可能会有问题,只是出问题的概率比较小:
a in set
这样的代码,如果 a 不在set里,那么只会进行第一步hash的比较,走不到第二三步的比较老静态图下对于
if x in list
这样的代码也不可用,基本用就会错。出错原因在3.2已经说明。5 PIR下如何解决
排查后目前框架中有多处set(Value)、dict[Value]以及
if value in list
这样的用法。目前可行且修改较小的一个方法方案如下,也是本PR进行的工作:if value in list
、list.index(x)
这样的代码,需要修改逻辑if x in list
这样的代码,但是目前没有想到办法能让这种代码raise error