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

[Pir]Fix Value eq error when using set #58896

Merged
merged 53 commits into from
Dec 18, 2023
Merged

Conversation

0x45f
Copy link
Contributor

@0x45f 0x45f commented Nov 10, 2023

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单测等多个地方都有类似的用法
image

之前为什么会加这样的逻辑呢?
因为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判断是否是同一个元素的步骤如下:

  1. 先判断两个对象的hash是否相同,如果相同则进行第二步
  2. 再判断两个对象是否是同一个对象,如果是同一个对象就认定为同一个元素;如果不同走第三步判断
  3. 再具体进行比较,会调用startkey对象和key对象的 == 方法(对于Variable和OpResult来说,== 的正确语义是在program中插入equal op,但是这个操作对于set判断来说是不正确的,所以只要走到这一步判断了就会出问题)

3.2 list

对于if x in list以及list.index(x)这样的python代码,会调用PyObject_RichCompareBool。PyObject_RichCompareBool内Py_EQ的时候

  • 先比较是否是同一个对象,如果不是同一个对象,进行第二步判断
  • 调用eq方法

4 老静态图类似情况说明

老静态图下因为Variable有name,所以很多地方使用的都是类似set(name)这种形式,set(name)这种用法是没有问题的。
对于set(Variable),老静态图也可能会有问题,只是出问题的概率比较小:

  • 类似 a in set这样的代码,如果 a 不在set里,那么只会进行第一步hash的比较,走不到第二三步的比较
  • 如果 a 在set里,那么第二步的比较就会返回结果了,也走不到第三步
  • 只有当两个Variable对象发生hash冲突的时候,才会真正走到第三步的判断。但是因为hash冲突的概率比较小,所以老静态图下出问题的概率很低,而且老静态图下Variable有name,老静态图下大部分的用法都是set(name)这样的用法。

老静态图下对于if x in list这样的代码也不可用,基本用就会错。出错原因在3.2已经说明。

5 PIR下如何解决

排查后目前框架中有多处set(Value)、dict[Value]以及 if value in list这样的用法。目前可行且修改较小的一个方法方案如下,也是本PR进行的工作:

  • 在框架内提供ValueSet和ValueDict两个数据结构,将set(Value)、dict[Value]相关的地方都替换为ValueSet和ValueDict
  • 对于if value in listlist.index(x)这样的代码,需要修改逻辑
  • 在python端禁掉value.__hash__函数,禁止用户使用set(Value)、dict[Value]。如果一定要用使用ValueSet和ValueDict
  • 禁止使用if x in list这样的代码,但是目前没有想到办法能让这种代码raise error

python/paddle/autograd/backward_utils.py Outdated Show resolved Hide resolved
python/paddle/autograd/backward_utils.py Outdated Show resolved Hide resolved
python/paddle/autograd/ir_backward.py Outdated Show resolved Hide resolved
python/paddle/autograd/ir_backward.py Outdated Show resolved Hide resolved
python/paddle/autograd/backward_utils.py Outdated Show resolved Hide resolved
python/paddle/autograd/backward_utils.py Outdated Show resolved Hide resolved
python/paddle/autograd/ir_backward.py Outdated Show resolved Hide resolved
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)
Copy link
Contributor

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的,因为对象类似不匹配

Copy link
Contributor

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 方法

Copy link
Contributor

@xiaoguoguo626807 xiaoguoguo626807 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 ir_backward

@0x45f 0x45f merged commit 97236a9 into PaddlePaddle:develop Dec 18, 2023
29 checks passed
@0x45f 0x45f deleted the fix-opresult-eq branch December 18, 2023 12:41
HermitSun pushed a commit to HermitSun/Paddle that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants