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

[CodeStyle][F821] refactor unittests utility function parameterized related code #47869

Merged
merged 9 commits into from
Nov 14, 2022

Conversation

SigureMo
Copy link
Member

@SigureMo SigureMo commented Nov 10, 2022

PR types

Others

PR changes

Others

Describe

目前单测里有几处使用了 parameterized 装饰器,有点类似 pytest.mark.parametrize 的功能,能够在 unittest.TestCase 上加装饰器直接写简单的 case,可读性其实比其他的会好很多(用过 pytest 应该都会很喜欢这个功能)

目前 unittest 并没有 built-in 的 parameterized 功能,paddle 现有的实现方式基本上每次使用都重新定义了一次(搜索 def parameterize 可以找到 5 处基本一样的重复代码,有两处是供其他同级别模块调用的),这些代码应当是来源于 https://github.com/wolever/parameterized/blob/master/parameterized/parameterized.py ,我觉得如果可以的话,可以考虑在测试依赖(python/unittest_py/requirements.txt)里引入 parameterized 库,避免重复定义

不过本 PR 不做这些啦,只是修复了 parameterized 相关单测里的 F821 问题,包括修复两个单测和清理没用的从 parameterized 库里搬过来的函数,下面 comments 详细说明下

本 PR 修复 F821 从 70 个 -> 43 个,共修复 27 个

Related links

@paddle-bot
Copy link

paddle-bot bot commented Nov 10, 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.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels Nov 10, 2022


@contextlib.contextmanager
def stgraph(func, *args):
Copy link
Member Author

@SigureMo SigureMo Nov 10, 2022

Choose a reason for hiding this comment

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

这是没用过的函数,这里参数表没有 x、axes、norm 这些参数,但下面用了(同样是 F821 检测出来的)

看起来貌似是从下面那个 test_fft_with_static_graph.py 里 copy 过来的(这里的 x、axes 等参数是吻合的),这些应该是 fft 那边专用的,distribution 应该用不了……

exe.run(sp)
[output] = exe.run(mp, feed={'input': x}, fetch_list=[output])
yield output
[output] = exe.run(mp, feed={'input': self.x}, fetch_list=[output])
Copy link
Member Author

Choose a reason for hiding this comment

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

这里 F821 检测出来是 place、x、axes 等参数未定义(忘记加 self. 了),而单测通过的原因是这里不小心 yield output(应该是从 stgraph 复制过来时候忘记删掉了) 导致这个函数返回一个 Generator 了,也就是单测执行期间,该函数所有语句并不会执行,所以修改前单测会通过

@@ -142,27 +142,6 @@ def default_name_func(func, num, p):
return base_name + name_suffix


def default_doc_func(func, num, p):
Copy link
Member Author

@SigureMo SigureMo Nov 10, 2022

Choose a reason for hiding this comment

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

这个是为 parameterize_func 提供的 doc 默认处理函数,但由于反正我们单测里这个 docstring 也不输出成文档,所以直接如 103 行那样,保持生成的多个 case 函数的 docstring 为原有函数的 docstring 即可

这里删除的主要原因是下面的 parameterized_argument_value_pairs、short_repr、to_text 都不存在(F821 问题),换言之,就是没从 https://github.com/wolever/parameterized/blob/master/parameterized/parameterized.py copy 过来,但这个处理对单测来说确实没啥用,所以就直接删了,这些函数就不 copy 过来了

@SigureMo SigureMo changed the title [WIP][CodeStyle][F821] refactor unittests utility function parameterized related code [CodeStyle][F821] refactor unittests utility function parameterized related code Nov 10, 2022
@luotao1 luotao1 self-assigned this Nov 11, 2022
@luotao1
Copy link
Contributor

luotao1 commented Nov 11, 2022

可以考虑在测试依赖(python/unittest_py/requirements.txt)里引入 parameterized 库


已经有了

@cxxly
Copy link
Contributor

cxxly commented Nov 11, 2022

非常好的建议,unittest的参数化测试插件parameterized已经引入到Paddle;可以先修复F821问题,感兴趣也可以把Paddle中自行实现的parameterized修改成使用parameterized插件

Copy link
Contributor

@cxxly cxxly 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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants