CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
[SOT] support inline call bool magic function #61790
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
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
bool_flag = True | ||
return VariableFactory.from_value( | ||
bool_flag, self.graph, ConstTracker(bool_flag) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
就目前来看 bool_flag
不需要单独作为一个变量,直接内联在使用处即可
不可以使用 ConstTracker,这个 True 是与 args 相关的,要用 DummyTracker
test/sot/test_builtin_bool.py
Outdated
return False | ||
|
||
|
||
def object_bool(obj): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
函数需要用 check_no_breakgraph 装饰确保没有打断,可参考其他单测的写法
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
再增加 operator.truth
和 bool
显式调用的 case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个单测在 Python 3.12 能跑过么?3.12 在 #61305 才初步支持,如果跑不过,在 test/sot/skip_files_py312 skip 一下
另外注意,跑单测时候记得加 STRICT_MODE=True 和 MIN_GRAPH_SIZE=0,CI 完整环境变量见 Paddle/paddle/scripts/paddle_build.sh Lines 991 to 995 in f8cf7e5
|
@@ -646,6 +646,12 @@ def call_function(self, /, *args, **kwargs): | |||
) | |||
assert isinstance(fn_var, VariableBase) | |||
return fn_var(*args) | |||
# If __bool__ method is absent, inline bool calls return True. | |||
elif magic_method.name == "__bool__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里再加一条 and not hasattr(arg_type, "__len__")
吧,不然其实也是有风险的
注释也贴上 Python 自己 dispatch 的源码链接
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
单测也加一些和 __len__
组合的情况,允许 breakgraph(就是不加 check_no_breakgraph),但结果需要是对的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
请问跑CI允许 breakgraph,是不是需要除了不加check_no_breakgraph, 但是要加上@strict_mode_guard(False);我本地调试如果设置STRICT_MODE=True,也不加@strict_mode_guard(False),单测还是会报错:FallbackError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
默认的 STRICT_MODE 是不允许 Fallback,按理说是不会 fallback 的,这里为什么会发生 fallback 呢?
@strict_mode_guard(False)
;
如非特殊 case,不允许加 strict_mode_guard(False)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
所以我本地单侧过不去的case是:如果一个只实现了__len__方法的类obj,在遇到if obj
class TestObjectWithLen:
def __init__(self,list):
self.list = list
def __len__(self):
return len(self.list)
也是不满足这个magic_method.name == "__bool__" and not hasattr(arg_type, "__len__")
特判的,还是会导致和上述一样的BreakGraphError
触发,接着触发FallbackError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
喔喔,这里有一个 BreakGraph -> Fallback 的转换,但没关系,这只影响 if obj
的单测 case,这样的 case 允许加 strict_mode_guard(False)
,但 bool(obj)
、operator.truth(obj)
应该不会 fallback,只会 breakgraph
另外,上面说的情况是存在 __len__
的情况,不存在的 __len__
的情况按理说不会 fallback 也不会 breakgraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
所以我本地单侧过不去的case是:如果一个只实现了__len__方法的类obj,在遇到if obj
没问题,我在发消息的时候还没有这条回复
test/sot/test_builtin_bool.py
Outdated
class TestBuiltinBool(TestCaseBase): | ||
def test_object(self): | ||
object = TestObject() | ||
self.assert_results(object_bool, object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不要用这种容易迷惑的名称,左边的 object_bool
明明是一个函数,可以改名 call_bool_in_cond
,右边的 object
与 builtin 的 object
名字冲突,建议改为 obj
test/sot/test_builtin_bool.py
Outdated
self.assert_results(object_bool, bool(object)) | ||
self.assert_results(object_bool, operator.truth(object)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这俩是在测啥?
test/sot/test_builtin_bool.py
Outdated
self.assert_results(test_bool, object) | ||
self.assert_results(test_operator_truth, object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同样,这里改成同风格的名字 call_bool_by_bool
和 call_bool_by_operator_truth
test/sot/test_builtin_bool.py
Outdated
return False | ||
|
||
|
||
@strict_mode_guard(False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strict_mode_guard 不是这样用的,可以参考其它单测,虽然这样可能生效
test/sot/skip_files_py312
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
以及 test/sot/skip_files_py312
和 python/paddle/jit/sot/opcode_translator/executor/variables/callable.py
怎么都 chmod 了?能不修改么?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是我本地检查pre-commit代码,没有权限才改的,我调试下改回来
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
其余目前没有问题了~
elif magic_method.name == "__bool__" and not hasattr( | ||
arg_type, "__len__" | ||
): | ||
return VariableFactory.from_value( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
奇怪,调试是有的呢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
明天上班我测试下,如果没问题的话可以找人豁免~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我试了下,确实没跑到,但单独跑一个 case 是可以跑到的,说明是 cache 导致的,因此在每个 case 都清理了 cache 就可以了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi, @diadestiny
|
PR types
Others
PR changes
Others
Description
当__bool__方法不存在时,在内联调用bool魔法函数时返回True