Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

adaptation to Python 3.9 #109

Merged
merged 21 commits into from
Jun 6, 2023
Merged

Conversation

gouzil
Copy link
Member

@gouzil gouzil commented Jun 1, 2023

增加了[DICT_UPDATE, DICT_MERGE, LIST_EXTEND, LIST_TO_TUPLE, IS_OP]

适配python3.9

closes #105

.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
def DICT_UPDATE(self, instr):
v = self.pop()
assert instr.argval > 0
dict.update(self._stack[-instr.arg].value, v)
Copy link
Member

@SigureMo SigureMo Jun 1, 2023

Choose a reason for hiding this comment

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

这里不应该直接操作 value,应该转发到 DictVariable 上的 update 方法(可参考 keys 等方法的实现)

而且该操作会有 SideEffects 产生,该机制尚未完成,可以在相应函数处记录 TODO

Copy link
Member Author

Choose a reason for hiding this comment

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

我尝试加了一个override_method_update

但是他会有如下报错, 可能是我哪个类型没写对吗,还是啥

symbolic_trace.utils.exceptions.InnerError: DummyTracker has no instructions

Copy link
Member

Choose a reason for hiding this comment

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

这里是由于 Tracker 机制,需要 Tracker 的组网输入找不到来源导致的(DummyTracker 是没有来源的),可参考 BUILD_xxx_UNPACK 的处理方式,先 get_wrapped_items 以确保数据是有 Tracker 的

Copy link
Member

Choose a reason for hiding this comment

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

可参考 https://github.com/2742195759/paddle-symbolic-trace/blob/add-contributing-md/CONTRIBUTING.md#guard-%E5%8F%8A%E7%BC%93%E5%AD%98%E6%9C%BA%E5%88%B6 配置下 Tracker 可视化来理解,分别在 python 3.8 和 python 3.9 可视化下,可以看到两者 Tracker 的差异

3.8:
out-py38

3.9(本 PR):
out-py39

修改后两者 Tracker 的图应该是一致的

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@gouzil gouzil changed the title add opcode [DICT_UPDATE, LIST_EXTEND, LIST_TO_TUPLE] add opcode [DICT_UPDATE, DICT_MERGE, LIST_EXTEND, LIST_TO_TUPLE] Jun 2, 2023
runs-on: ubuntu-latest
name: Run unit tests and examples
strategy:
fail-fast: false
Copy link
Member Author

@gouzil gouzil Jun 2, 2023

Choose a reason for hiding this comment

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

如果任何矩阵作业失败,GitHub 将不会取消所有正在进行的作业, 说明链接

@gouzil gouzil changed the title add opcode [DICT_UPDATE, DICT_MERGE, LIST_EXTEND, LIST_TO_TUPLE] Adapter python3.9 Jun 2, 2023
@gouzil gouzil requested a review from SigureMo June 2, 2023 10:31
@@ -24,6 +24,9 @@ def get_paddle_api():
for fn_name in getattr(module, "__all__", []):
fn = getattr(module, fn_name)
if inspect.isfunction(fn):
# NOTE: Maybe add a whitelist list
if fn.__name__ == "in_dygraph_mode":
continue
Copy link
Member

Choose a reason for hiding this comment

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

同步一下最新的 develop,#108 已修复

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

dict.update(retval, self._stack[-instr.arg].get_wrapped_items())
self._stack[-instr.arg] = DictVariable(
retval, self._graph, DummyTracker(dict_value)
)
Copy link
Member

Choose a reason for hiding this comment

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

返回的 DictVariable 与原来的是不共享数据的吗?这是符合预期的吗?

Copy link
Member

Choose a reason for hiding this comment

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

还是不建议在 OpCodeExecutor 里直接操作 value,这也做不到可复用,比如用户调用了 d1.update(d2),仍然需要重新写一遍相同的逻辑,list.extend 有同样的问题

)

# Like DICT_UPDATE but raises an exception for duplicate keys.
DICT_MERGE = DICT_UPDATE
Copy link
Member

Choose a reason for hiding this comment

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

but raises an exception for duplicate keys.

这一点是如何体现的呢?另外新增的字节码也应该增加单测来确保正确性

Copy link
Member Author

@gouzil gouzil Jun 2, 2023

Choose a reason for hiding this comment

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

DICT_MERGE文档, 这个test_10_build_unpack就会测试到,以及paddle/nn/layer/layers下的_build_once也会使用到

测试值为{"a": a, "b": b}, {"a": c, "b": d}

按照之前的想法如果key重复会直接被CALL_FUNCTION_EX拦截, 这样其实是不太合理的

  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_executor.py", line 306, in run
    is_stop = self.step(cur_instr)
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_executor.py", line 314, in step
    return getattr(self, instr.opname)(instr)  # run single step.
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_executor.py", line 669, in CALL_FUNCTION_EX
    ret = fn(*args, **kwargs)
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/variables.py", line 718, in __call__
    return self.call_function(*args, **kwargs)
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/variables.py", line 795, in call_function
    inline_executor = OpcodeInlineExecutor(self, *args, **kwargs)
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_inline_executor.py", line 47, in __init__
    self._prepare_locals(*args, **kwargs)
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_inline_executor.py", line 54, in _prepare_locals
    bound_args = sig.bind(*args, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/inspect.py", line 3045, in bind
    return self._bind(args, kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/inspect.py", line 3015, in _bind
    raise TypeError('missing a required argument: {arg!r}'. \
TypeError: missing a required argument: 'c'

已修复为:

  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_executor.py", line 306, in run
    is_stop = self.step(cur_instr)
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_executor.py", line 314, in step
    return getattr(self, instr.opname)(instr)  # run single step.
  File "/Users/gouzi/Documents/git/paddle-symbolic-trace/symbolic_trace/opcode_translator/executor/opcode_executor.py", line 948, in DICT_MERGE
    assert result == None, f"Repeat key: {key}"
AssertionError: Repeat key: a

Copy link
Member

Choose a reason for hiding this comment

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

这里报 InnerError 吧,我们之后会统一规范下报错体系书写方式 #114

另外,报错信息和运行时错误保持一致:

TypeError: __main__.foo() got multiple values for keyword argument 'a'

TypeError: __main__.foo() 这部分我们可以暂时先不考虑,后面的报错信息保持一致吧

Copy link
Member

Choose a reason for hiding this comment

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

其余 LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

list_value = self.pop()
self.push(
TupleVariable(
list_value.value, self._graph, DummyTracker([instr.argval])
Copy link
Member

Choose a reason for hiding this comment

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

这里同样可以 get_wrapped_items 一下确保 Tracker 保留

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@SigureMo
Copy link
Member

SigureMo commented Jun 5, 2023

#110 已 merge,麻烦解决下冲突~

…o add_opcode_list_dict

# Conflicts:
#	symbolic_trace/opcode_translator/executor/variables.py
@gouzil
Copy link
Member Author

gouzil commented Jun 5, 2023

#110 已 merge,麻烦解决下冲突~

Done

@gouzil gouzil requested a review from SigureMo June 6, 2023 08:22
2742195759
2742195759 previously approved these changes Jun 6, 2023
Copy link
Collaborator

@2742195759 2742195759 left a comment

Choose a reason for hiding this comment

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

LGTM

# It will only be 0 or 1
assert instr.argval == 0 or instr.argval == 1
left = self.pop()
right = self.pop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里pop的顺序是不是有点问题?先pop出来的应该是right元素吧

Copy link
Member

@SigureMo SigureMo Jun 6, 2023

Choose a reason for hiding this comment

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

@gouzil 编写字节码模拟执行代码时候可以参考 cpython 的 ceval.c,我们在实现的时候也是优先参考和对齐它的:

https://github.com/python/cpython/blob/3.9/Python/ceval.c#L3019-L3031

(虽然 IS_OP 左右应该没太大问题,但最好还是对齐比较好)

Copy link
Member Author

Choose a reason for hiding this comment

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

是的是的,已修复,这个跟COMPARE_OP其实还是挺像的

Copy link
Member

Choose a reason for hiding this comment

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

IS_OP 应该是 3.9 版本从 COMPARE_OP 里抽离出来的部分操作,3.8 的 is/is not 是在 COMPARE_OP 里的

@SigureMo SigureMo changed the title Adapter python3.9 adaptation to Python 3.9 Jun 6, 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.

LGTM

@gouzil gouzil requested review from 0x45f and 2742195759 June 6, 2023 12:09
Copy link
Collaborator

@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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.9 版本支持
4 participants