[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

@@ -78,17 +78,17 @@ def __init__(
        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

        return self.graph.call_tensor_method(
-            '__getitem__',
+            'getitem',


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

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

sot/opcode_translator/executor/variables/base.py

@@ -291,22 +291,6 @@ def __repr__(self):
    def __str__(self):
        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

@@ -188,17 +188,17 @@ def from_value(value: Any, graph: FunctionGraph | None, tracker: Tracker):
 class TupleVariable(ContainerVariable):
    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

        '''
-        why __setitem__ is ok:
+        why __setitem__ or setitem is ok:


注释里 __setitem__ 就不需要了吧

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

sot/opcode_translator/executor/variables/container.py

@@ -142,18 +142,25 @@ def f(x = [t0, t1])
                f"[{self.__class__.__name__}]: received {key} as key."
            )

-        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

@@ -51,6 +51,7 @@
    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[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

paddle-bot bot commented Jun 24, 2023

SigureMo Jun 24, 2023

SigureMo Jun 24, 2023

SigureMo Jun 24, 2023

SigureMo Jun 24, 2023

gouzil Jun 25, 2023

SigureMo Jun 25, 2023

SigureMo Jun 25, 2023

gouzil commented Jun 25, 2023

SigureMo left a comment

SigureMo Jun 25, 2023

gouzil Jun 26, 2023

SigureMo Jun 25, 2023

gouzil Jun 26, 2023

SigureMo Jun 26, 2023

gouzil Jun 26, 2023

SigureMo Jun 26, 2023

gouzil Jun 26, 2023

SigureMo Jun 26, 2023

gouzil Jun 26, 2023

SigureMo Jun 26, 2023

gouzil Jun 26, 2023

SigureMo Jun 26, 2023

gouzil Jun 26, 2023

SigureMo left a comment

2742195759 left a comment

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

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

Conversation

gouzil commented Jun 24, 2023 • edited Loading

paddle-bot bot commented Jun 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouzil commented Jun 25, 2023

SigureMo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SigureMo left a comment

Choose a reason for hiding this comment

2742195759 left a comment

Choose a reason for hiding this comment

[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