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

implement VariableBase #68

Merged
merged 14 commits into from
Sep 5, 2023
Merged

implement VariableBase #68

merged 14 commits into from
Sep 5, 2023

Conversation

zrr1999
Copy link
Contributor

@zrr1999 zrr1999 commented Sep 3, 2023

本 PR 的目标是使用类似 PaddleSOT Variable 体系替换掉原本的 Python 普通变量。

Variable类 的实现源于 https://github.com/PFCCLab/paddlefx/pull/54。

从 PaddleSOT 移植的模块

  1. VariableStack 是 PaddleSOT 中相对独立的模块,可以直接复用
  2. Dispatcher 模块

@zrr1999
Copy link
Contributor Author

zrr1999 commented Sep 3, 2023

@jzhang533 @gglin001 @SigureMo 麻烦看一下实现方案的问题

self.f_locals: dict[str, VariableBase] = {}
self.f_builtins: dict[str, VariableBase] = {}
# TODO(zrr1999): self.stack should be a stack of VariableBase
self.stack: VariableStack[VariableBase | Any] = VariableStack()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any 是?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这块因为还没有实际修改,stack目标是全员Variable,但是现在是全员不是Vriable

elif name in self.frame.f_builtins:
self.push(self.frame.f_builtins[name])
if name in self.f_globals:
self.stack.push(self.f_globals[name].value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

咦?这里为啥往栈上放了 value 而不是 variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这块也是因为其他部分代码还没有修改,比较暂时的写法

@@ -12,23 +12,25 @@
from .bytecode_transformation import Instruction, create_instruction
from .output_graph import OutputGraph
from .proxy import Attribute, Proxy
from .variable_stack import VariableStack
Copy link
Collaborator

Choose a reason for hiding this comment

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

可以将 Variable 的使用放在 src/paddlefx/pyeval.py 里面吗(替换掉 src/paddlefx/symvar.pySymvar)? 我想这里的 src/paddlefx/translator.py 被弃用了
https://github.com/PFCCLab/paddlefx/blob/main/src/paddlefx/pyeval.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,已经在src/paddlefx/pyeval.py利用Variable替换了SymVar

Copy link
Collaborator

@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.

LGTMeow 🐾

@SigureMo SigureMo merged commit fb3520f into PFCCLab:main Sep 5, 2023
6 checks passed
@jzhang533
Copy link
Contributor

看起来这个PR 破坏了原来一些能运行的情况。
比如: https://github.com/PFCCLab/paddlefx/blob/main/examples/targets/target_2_func.py
我们是不是考虑在CI里对这些做保护?
@gglin001 @zrr1999

@gglin001
Copy link
Collaborator

看起来这个PR 破坏了原来一些能运行的情况。 比如: https://github.com/PFCCLab/paddlefx/blob/main/examples/targets/target_2_func.py 我们是不是考虑在CI里对这些做保护? @gglin001 @zrr1999

有了 VariableBase 的支持, 现在应该可以支持更多的 case, 这些典型case可以加到 tests/ 目录下

@jzhang533
Copy link
Contributor

嗯,我还在想接下来怎么样来实现lower到另外一个后端来执行。

@zrr1999
Copy link
Contributor Author

zrr1999 commented Sep 27, 2023

看起来这个PR 破坏了原来一些能运行的情况。 比如: https://github.com/PFCCLab/paddlefx/blob/main/examples/targets/target_2_func.py 我们是不是考虑在CI里对这些做保护? @gglin001 @zrr1999

看起来这个PR 破坏了原来一些能运行的情况。 比如: https://github.com/PFCCLab/paddlefx/blob/main/examples/targets/target_2_func.py 我们是不是考虑在CI里对这些做保护? @gglin001 @zrr1999

这里的问题看起来是因为还没有对f_local的变量进行分类现在全部都是VariableBase,应该把一部分改成CallableVariable,我稍后修复一下顺便加一下单测

@zrr1999 zrr1999 mentioned this pull request Sep 27, 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.

4 participants