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] Add CI for self.assertTrue(np.allclose(...)) #45126

Merged
merged 15 commits into from
Aug 16, 2022

Conversation

Yulv-git
Copy link
Contributor

@Yulv-git Yulv-git commented Aug 14, 2022

PR types

Others

PR changes

Others

Describe

添加阻止使用 self.assertTrue(np.allclose(...)) 或 self.assertEqual(np.allclose(...)) 的 CI。

问题来源于 Recommend to use np.testing.assert_allclose instead of assertTrue(np.allclose(...)) #44641
RFC:[WIP, Don't review][CodeStyle] 单测报错信息优化 RFC community#198

@paddle-bot
Copy link

paddle-bot bot commented Aug 14, 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 Aug 14, 2022
Copy link
Contributor Author

@Yulv-git Yulv-git left a comment

Choose a reason for hiding this comment

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

待测试并完善新增的CI脚本后,删除测试样例文件(python/paddle/fluid/tests/unittests/CI_test文件夹及其下的文件)。

Copy link
Contributor Author

@Yulv-git Yulv-git left a comment

Choose a reason for hiding this comment

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

grep -zoEnr "self\.assert(True|Equal)\(\s\s*(np|numpy)\.(allclose|array_equal)" ./

./python/paddle/fluid/contrib/slim/tests/test_weight_quantization_mobilenetv1.py:1:self.assertTrue( np.allclose./python/paddle/fluid/contrib/tests/test_model_cast_to_bf16.py:1:self.assertTrue( np.allclose./python/paddle/fluid/contrib/tests/test_model_cast_to_bf16.py:1:self.assertTrue( np.allclose./python/paddle/fluid/contrib/tests/test_weight_decay_extend.py:1:self.assertTrue( np.allclose./python/paddle/fluid/tests/custom_op/test_custom_tanh_double_grad.py:1:self.assertTrue( np.allclose./python/paddle/fluid/tests/custom_op/test_custom_tanh_double_grad.py:1:self.assertTrue( np.allclose./python/paddle/fluid/tests/custom_op/test_custom_tanh_double_grad.py:1:self.assertTrue( np.allclose./python/paddle/fluid/tests/test_if_else_op.py:1:self.assertTrue( np.allclose./python/paddle/fluid/tests/unittests/benchmark.py:1:self.assertTrue( np.allclose./python/paddle/fluid/tests/unittests/CI_test/test_dropout_op.py:1:self.assertTrue( np.allclose
...
./python/paddle/fluid/tests/unittests/xpu/test_batch_norm_op_xpu.py:1:self.assertEqual( np.allclose./python/paddle/tests/test_async_read_write.py:1:self.assertTrue( np.allclose

@luotao1 luotao1 self-assigned this Aug 15, 2022
@SigureMo SigureMo force-pushed the CI-assert-allclose branch 3 times, most recently from 20a46a0 to 1932eae Compare August 15, 2022 19:36
Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

删除几个测试文件即可。

tools/check_file_diff_approvals.sh Outdated Show resolved Hide resolved
tools/check_file_diff_approvals.sh Show resolved Hide resolved
tools/check_file_diff_approvals.sh Outdated Show resolved Hide resolved
@@ -259,6 +259,12 @@ if [ "${EMPTY_GRAD_OP_REGISTERED}" != "" ] && [ "${GIT_PT_ID}" != "" ]; then
check_approval 1 43953930 46782768 22165420 22361972
fi

INVALID_UNITTEST_ASSERT_CHECK=`echo "$ALL_ADDED_LINES" | grep -zoE '\+\s+self\.assert(True|Equal)\((\s*\+\s*)?(np|numpy)\.(allclose|array_equal)[^+]*' || true`
Copy link
Contributor

Choose a reason for hiding this comment

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

照理说,这行应该会触发PR-CI-Approval的

Copy link
Member

@SigureMo SigureMo Aug 16, 2022

Choose a reason for hiding this comment

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

因为修改后的正则为了显示更加友好,把整行都给匹配出来。它要求以 + 开头且后接一系列空格之后再接 self.assertXxx,相对于原来是更加严格的。

+     self.assertTrue(           <---- 这一行前面要求必须是空格开头
+                np.allclose(...

因此它现在是不会将这个脚本自身内容视为关键词触发,因为这个脚本里 self.assertTrue( 前面并不是空格

@luotao1 luotao1 changed the title [WIP][CodeStyle] Add CI for self.assertTrue(np.allclose(...)) [CodeStyle] Add CI for self.assertTrue(np.allclose(...)) Aug 16, 2022
Copy link
Contributor

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

3 participants