[Dispatch] use dispatch replace `getitem`, `setitem`, `delitem` and `bool` #198

gouzil · 2023-06-24T15:49:00Z

operator.getitem -> VariableBase/TensorVariable/ListVariable/TupleVariable/DictVariable.__getitem__
operator.setitem -> VariableBase/TensorVariable/ListVariable/TupleVariable/DictVariable.__setitem__
operator.delitem -> ListVariable/TupleVariable/DictVariable.__delitem__
bool -> ConstantVariable.__bool__
整理单测
补充 use dispatch instead of monkey patch #182 遗漏单测

close #194

paddle-bot · 2023-06-24T15:49:05Z

Thanks for your contribution!

SigureMo · 2023-06-24T16:33:14Z

sot/opcode_translator/executor/variables/container.py

        self.value = val_list

    def get_value(self):
-        return [self[i].get_value() for i in range(len(self))]


为了方便，这里就不要改了

def __getitem__(self, idx): # 看情况是只给容器加还是 VariableBase 加 return self.getitem(idx)

与 getattr 不同，getattr 只要有 . 就会进入，很容易出问题，但 __getitem__ 不会

SigureMo · 2023-06-24T16:33:35Z

sot/opcode_translator/executor/variables/basic.py

+    def getitem(self, key):
        return self.graph.call_tensor_method(
-            '__getitem__',
+            'getitem',


这里为什么要改？这样还会对吗？

SigureMo · 2023-06-24T16:36:22Z

sot/opcode_translator/executor/variables/base.py

        return self.__repr__()

-    def __getitem__(self, item):
-        # TODO: Remove this function after we use builtin dispatcher instead


这个函数不一定能删吧，这里只是我早期认为可以直接替换掉，实际上可能不是这样

SigureMo · 2023-06-24T16:41:04Z

tests/test_05_dict.py

#182 我忘记加 binary op、unary op 相关的了，可以在 test_14_operators 加一下 operator.add(x, y) 这类的

需不需要把list那些也移动到test_14_operators，比如test_04_list里的list_getitem

sot/opcode_translator/executor/variable_dispatch.py

SigureMo · 2023-06-25T02:11:18Z

sot/opcode_translator/executor/variable_dispatch.py

+    operator.getitem,
+    (
+        "VariableBase | TensorVariable | ListVariable | TupleVariable | DictVariable",
+        "int | str | TensorVariable",


有没有可能是 slice？还有 tuple？

slice: a[1:10:3]

tuple: a[10, 11]

还有其他类型数据可不可以呢？如果都可以的话这里只加这几种类型是有问题的

Co-authored-by: Nyakku Shigure <sigure.qaq@gmail.com>

gouzil · 2023-06-25T07:50:40Z

单测的内容我会最后一起整理

SigureMo

主体逻辑看起来是没啥问题了

SigureMo · 2023-06-25T09:33:32Z

sot/opcode_translator/executor/variable_dispatch.py

+    operator.getitem,
+    (
+        "VariableBase | TensorVariable | ContainerVariable",
+        "int | str | TensorVariable | slice",


这里 TensorVariable 是不是会出问题呢？现在是不是没有相关的 case，这个问题算是一个遗留问题，和 66 行的 getattr 应该算同一个问题，在 BINARY_SUBSCR 里我们直接将 value 取出来传进 getitem 了，所以 tensor1[tensor2] 假如 tensor2 是一个中间变量的话（value 为 None），是会有问题的，这里应该将其 Variable 传入 getitem，并在 getitem 里做取 value 等操作

这个 PR 先不改这个问题吧，可以下个 PR 统一修改下 getitem、getattr 的问题，这个 PR 记录一下 TODO 即可

SigureMo · 2023-06-25T09:37:42Z

sot/opcode_translator/executor/variables/container.py

    def __init__(
        self,
-        val_tuple: list[VariableBase],
+        val_tuple: list[VariableBase] | tuple[VariableBase],


这里就不要允许传入 list[VariableBase] 了，直接强制 tuple[VariableBase] 吧，应该没有什么需求是传入 list？

…test

…o replace_magic_method # Conflicts: # sot/opcode_translator/executor/variable_dispatch.py

SigureMo · 2023-06-26T11:23:27Z

sot/opcode_translator/executor/variable_dispatch.py

+Dispatcher.register(
+    operator.getitem,
+    (
+        "VariableBase | TensorVariable | ContainerVariable",


这里 VariableBase 已经包含了 TensorVariable | ContainerVariable，所以后面的需要删掉

SigureMo · 2023-06-26T11:26:15Z

sot/opcode_translator/executor/variables/container.py

+    def setitem(self, key, value):
        '''
-        why __setitem__ is ok:
+        why __setitem__ or setitem is ok:


注释里 __setitem__ 就不需要了吧

SigureMo · 2023-06-26T11:27:23Z

sot/opcode_translator/executor/variables/container.py

            )

-        if not isinstance(value, VariableBase):
+        if isinstance(value, TensorVariable):


什么情况会传进来 Tensor 呢？对 Tensor get_value 不对吧

test_04的list_setitem_tensor

那也不应该 get_value，这里不应该修改，这是没有支持 SideEffects 导致的，单测先禁掉加 TODO 即可

SigureMo · 2023-06-26T11:29:53Z

sot/utils/magic_methods.py

    operator.pow: ("__pow__", "__rpow__"),
    operator.rshift: ("__rshift__", "__rrshift__"),
    operator.sub: ("__sub__", "__rsub__"),
+    operator.setitem: ("__setitem__", None),


setitem 不是 binary op 吧，这个是在什么 case 使用到的呢？

…add todo

SigureMo

LGTM～

2742195759

LGTM

[executor] replace getitem and ConstantVariable.__bool__

82d41ad

paddle-bot bot added contributor External developers status: proposed labels Jun 24, 2023

SigureMo reviewed Jun 24, 2023

View reviewed changes

SigureMo changed the title ~~use dispatch replace getitem、setitem、delitem、bool~~ use dispatch replace getitem, setitem, delitem and bool Jun 24, 2023

SigureMo changed the title ~~use dispatch replace getitem, setitem, delitem and bool~~ [Dispatch] use dispatch replace getitem, setitem, delitem and bool Jun 24, 2023

SigureMo mentioned this pull request Jun 24, 2023

🐾 PaddleSOT 快乐喵喵开源任务 🚀 #133

Closed

[variables] RollBACK basic and container; [variables] add __getitem__

239bcec

SigureMo reviewed Jun 25, 2023

View reviewed changes

gouzil and others added 2 commits June 25, 2023 10:36

[executor] Dispatcher clean and add ContainerVariable

7a1bdd9

Co-authored-by: Nyakku Shigure <sigure.qaq@gmail.com>

[variables] support slice

0b27dcd

[test] add tuple slice test

8855b0e

SigureMo reviewed Jun 25, 2023

View reviewed changes

gouzil added 6 commits June 26, 2023 00:00

[variables] fix TupleVariable and support setitem;[test] add setitem …

f539545

…test

Merge branch 'develop' of github.com:gouzil/paddle-symbolic-trace int…

5129c96

…o replace_magic_method # Conflicts: # sot/opcode_translator/executor/variable_dispatch.py

[executor] suport delitem;[tests] add delitem test

c595f30

[executor] add todo

3c73305

[variables] fix list getitem;[tests] move to operators

d828024

[tests] supplementary testing

32dc2dc

gouzil requested a review from SigureMo June 26, 2023 09:03

SigureMo reviewed Jun 26, 2023

View reviewed changes

[variable] fix list setitem; [magic_methods] rm __setitem__; [tests] …

61bb321

…add todo

gouzil requested a review from SigureMo June 26, 2023 12:24

SigureMo approved these changes Jun 26, 2023

View reviewed changes

2742195759 approved these changes Jun 27, 2023

View reviewed changes

2742195759 merged commit 68121ad into PaddlePaddle:develop Jun 27, 2023

SigureMo mentioned this pull request Aug 2, 2023

🪄 PaddleSOT 喵喵大入侵计划 💥 #305

Closed

gouzil mentioned this pull request Oct 17, 2023

【社区治理】gouzil 发起 Committer 身份申请 PaddlePaddle/Paddle#58151

Closed

[Dispatch] use dispatch replace getitem, setitem, delitem and bool #198

[Dispatch] use dispatch replace getitem, setitem, delitem and bool #198

Uh oh!

Conversation

gouzil commented Jun 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paddle-bot bot commented Jun 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouzil commented Jun 25, 2023

Uh oh!

SigureMo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SigureMo left a comment

Choose a reason for hiding this comment

Uh oh!

2742195759 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Dispatch] use dispatch replace `getitem`, `setitem`, `delitem` and `bool` #198

[Dispatch] use dispatch replace `getitem`, `setitem`, `delitem` and `bool` #198

gouzil commented Jun 24, 2023 •

edited

Loading