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

[ Dy2Static ]Change NameVisitor in while to FunctionScopeAnalysis #44155

Merged

Conversation

2742195759
Copy link
Contributor

@2742195759 2742195759 commented Jul 7, 2022

PR types

Others

PR changes

Others

Describe

  1. Change NameVisitor in while to FunctionScopeAnalysis
  2. Add CreateVariableTransformer to solve non-local not bound
  3. Unify the 3 type of for to 1 type. for A in B
  4. Support non-nested zip / range / enumerate
  5. Support step is a variable in range()
  6. Fix [ i for i in range(10) ] bugs.

TODO:

  1. why pd_range get wrong answer.

@paddle-bot
Copy link

paddle-bot bot commented Jul 7, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@2742195759 2742195759 changed the title Change NameVisitor in while to FunctionScopeAnalysis [ Dy2Static ]Change NameVisitor in while to FunctionScopeAnalysis Jul 7, 2022
var1 = FOR_ITER_TUPLE_PREFIX_x[0]
var2 = FOR_ITER_TUPLE_PREFIX_x[1][0]
var3 = FOR_ITER_TUPLE_PREFIX_x[1][1]
class ForLoopTuplePreTransformer(gast.NodeTransformer):
Copy link
Contributor

Choose a reason for hiding this comment

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

这里要继承BaseTransformer吧,不然报错会截断

for i, element in enumerate(node.elts):
ret += self.tuple_to_stmts(node.elts[i], tuple_name, idx + [i])
if not isinstance(node, (gast.Tuple, gast.List)):
return 1
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number? 这里是用来表示structure的?比如 [1, [1,1] , 1] ? 如果是的话,这个函数加下注释吧

# Non-enumrate case:
tuple_name = unique_name.generate(FOR_ITER_TUPLE_PREFIX)
self.generic_visit(node)
if self.is_for_iter(node):
Copy link
Contributor

Choose a reason for hiding this comment

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

这个if是不是可以移除了?我看函数里就是简单的判断


def is_for_enumerate_iter(self, for_node):
def is_for_iter(self, for_node):
Copy link
Contributor

Choose a reason for hiding this comment

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

这个函数是不是没有什么用了?

is_builtin_len = eval("is_builtin_len({})".format(func_str))
is_builtin_zip = eval("is_builtin_zip({})".format(func_str))
return is_builtin and not is_builtin_len and not is_builtin_zip
is_need_convert = func_str in need_convert_builtin_func_list
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_need_convert = func_str in need_convert_builtin_func_list
need_convert = func_str in need_convert_builtin_func_list

不需要 is 前缀的

@@ -34,6 +34,7 @@
from paddle.fluid.dygraph.dygraph_to_static.utils import create_nonlocal_stmt_nodes
from paddle.fluid.dygraph.dygraph_to_static.utils import create_get_args_node, create_set_args_node
from paddle.fluid.dygraph.dygraph_to_static.base_transformer import BaseTransformer
from paddle.fluid.dygraph.dygraph_to_static.utils import FOR_ITER_INDEX_PREFIX, FOR_ITER_TUPLE_PREFIX, FOR_ITER_TUPLE_INDEX_PREFIX, FOR_ITER_VAR_LEN_PREFIX, FOR_ITER_VAR_NAME_PREFIX, FOR_ITER_ZIP_TO_LIST_PREFIX, FOR_ITER_TARGET_PREFIX, FOR_ITER_ITERATOR_PREFIX
Copy link
Contributor

Choose a reason for hiding this comment

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

下个PR可以把所有的全局变量放到一个class里来管理,后续import时只需要导入一个就可以了。

@@ -82,6 +82,8 @@ def visit(self, node):

FOR_ITER_INDEX_PREFIX = '__for_loop_var_index'
FOR_ITER_TUPLE_PREFIX = '__for_loop_iter_tuple'
FOR_ITER_TARGET_PREFIX = '__for_loop_iter_target'
Copy link
Contributor

Choose a reason for hiding this comment

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

下个PR我们可以规范下这些前缀名字,有的太长了,不利于可读性

In this case, `i` will not created in FunctionScope.
We don't collect `i` by not calling generic_visit.
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

这里应该是直接 return node 吧,如果是pass的话,node会丢失吧?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是一个visitor,不是transformer,不会丢失的。

4. UndefinedVar = value # create a variable
"""
from paddle.fluid.dygraph.dygraph_to_static.utils import UndefinedVar
from paddle.fluid.dygraph.dygraph_to_static.utils import UndefinedVar, create_undefined_variable
Copy link
Contributor

Choose a reason for hiding this comment

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

语句有重复的

@@ -26,7 +26,7 @@
from .convert_operators import convert_print as Print # noqa: F401
from .convert_operators import convert_shape as Shape # noqa: F401
from .convert_operators import convert_while_loop as While # noqa: F401

from .convert_operators import unpack_by_structure, indexable # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from .convert_operators import unpack_by_structure, indexable # noqa: F401
from .convert_operators import unpack_by_structure as Unpack # noqa: F401
from .convert_operators import indexable as Index # noqa: F401

暴露的API尽可能一个单词,且首字母大写

Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

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

LGTM

@2742195759 2742195759 merged commit c5c6026 into PaddlePaddle:develop Jul 12, 2022
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.

2 participants