From ba7e2a9f5bf497ff921e8d5b668f01c4fabce40f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=20Wei=20=28=E4=BB=BB=E5=8D=AB=29?= Date: Thu, 24 Jun 2021 10:22:44 +0800 Subject: [PATCH] remove the tricks for `paddle.fluid.layers.ops.func` (#33731) * refactor check_pr_approval, allow using github login-id 2. remove the tricks for paddle.fluid.layers.ops.func * add testcases * simplify the test data, and added to file diff approvals * remove a approver * test_print_signatrues runs on a simple pipeline, no paddle installed * testcases for print_signatrures and sampcd . python3 only. * remove unused import directives * remove unused import directives --- tools/check_api_approvals.sh | 24 ++---- tools/check_file_diff_approvals.sh | 4 + tools/check_pr_approval.py | 12 ++- tools/print_signatures.py | 14 +--- tools/sampcd_processor.py | 1 - tools/test_check_pr_approval.py | 120 +++++++++++++++++++++++++++++ tools/test_print_signatures.py | 49 +++++------- tools/test_sampcd_processor.py | 3 - 8 files changed, 164 insertions(+), 63 deletions(-) create mode 100644 tools/test_check_pr_approval.py diff --git a/tools/check_api_approvals.sh b/tools/check_api_approvals.sh index 74ef549f3d3..40a0a618fb0 100644 --- a/tools/check_api_approvals.sh +++ b/tools/check_api_approvals.sh @@ -38,11 +38,7 @@ function add_failed(){ api_spec_diff=`python ${PADDLE_ROOT}/tools/diff_api.py ${PADDLE_ROOT}/paddle/fluid/API_DEV.spec.api ${PADDLE_ROOT}/paddle/fluid/API_PR.spec.api` -ops_func_in_diff=$(echo ${api_spec_diff} | grep '\bpaddle\.fluid\.layers\.ops\.func\b') -linenum=$(echo ${api_spec_diff} | wc -l | sed 's/[[:space:]]//g') -if [ "${linenum}" = "3" -a "${ops_func_in_diff}" != "" ] ; then - echo "skip paddle.fluid.layers.ops.func" -elif [ "$api_spec_diff" != "" ]; then +if [ "$api_spec_diff" != "" ]; then echo_line="You must have one RD (XiaoguangHu01 or lanxianghit) approval for API change.\n" echo_line="${echo_line} and one TPM approval for API change: \n" echo_line="${echo_line} jzhang533/ZhangJun, dingjiaweiww/DingJiaWei, Heeenrrry/LiKunLun, TCChenlong/ChenLong for general APIs\n" @@ -54,10 +50,7 @@ elif [ "$api_spec_diff" != "" ]; then fi api_doc_spec_diff=`python ${PADDLE_ROOT}/tools/diff_api.py ${PADDLE_ROOT}/paddle/fluid/API_DEV.spec.doc ${PADDLE_ROOT}/paddle/fluid/API_PR.spec.doc` -linenum=$(echo ${api_doc_spec_diff} | wc -l | sed 's/[[:space:]]//g') -if [ "${linenum}" = "3" -a "${ops_func_in_diff}" != "" ] ; then - echo "skip paddle.fluid.layers.ops.func for doc diff" -elif [ "$api_doc_spec_diff" != "" ]; then +if [ "$api_doc_spec_diff" != "" ]; then echo_line="You must have one TPM approval for API documents change: \n" echo_line="${echo_line} jzhang533/ZhangJun, dingjiaweiww/DingJiaWei, Heeenrrry/LiKunLun, TCChenlong/ChenLong for general API docs\n" echo_line="${echo_line} PangHua/XiangHui for distributed related API docs\n" @@ -76,8 +69,8 @@ fi op_type_spec_diff=`python ${PADDLE_ROOT}/tools/check_op_register_type.py ${PADDLE_ROOT}/paddle/fluid/OP_TYPE_DEV.spec ${PADDLE_ROOT}/paddle/fluid/OP_TYPE_PR.spec` if [ "$op_type_spec_diff" != "" ]; then - echo_line="You must have one RD (Aurelius84 (Recommend) or liym27 or zhhsplendid)approval for the data_type registration of new operator. More data_type of new operator should be registered in your PR. Please make sure that both float/double (or int/int64_t) have been registered.\n For more details, please click [https://github.com/PaddlePaddle/Paddle/wiki/Data-types-of-generic-Op-must-be-fully-registered].\n" - check_approval 1 9j301846 33742067 7913861 + echo_line="You must have one RD (Aurelius84 (Recommend) or zhhsplendid)approval for the data_type registration of new operator. More data_type of new operator should be registered in your PR. Please make sure that both float/double (or int/int64_t) have been registered.\n For more details, please click [https://github.com/PaddlePaddle/Paddle/wiki/Data-types-of-generic-Op-must-be-fully-registered].\n" + check_approval 1 9301846 7913861 fi op_desc_diff=`python ${PADDLE_ROOT}/tools/check_op_desc.py ${PADDLE_ROOT}/paddle/fluid/OP_DESC_DEV.spec ${PADDLE_ROOT}/paddle/fluid/OP_DESC_PR.spec` @@ -100,12 +93,9 @@ if [ -n "${echo_list}" ];then echo "There are ${failed_num} approved errors." echo "****************" - # L40 L48 L62 has fetch the result out. - if [ "${api_spec_diff}" != "" ] ; then - echo "api_spec_diff: ${api_spec_diff}" - fi - if [ "${api_doc_spec_diff}" != "" ] ; then - echo "api_doc_spec_diff: ${api_doc_spec_diff}" + # L40 L48 L62 has fetch the result out, but there are splitted. + if [ "${api_spec_diff}" != "" -o "${api_doc_spec_diff}" != "" ] ; then + python ${PADDLE_ROOT}/tools/diff_api.py ${PADDLE_ROOT}/paddle/fluid/API_DEV.spec ${PADDLE_ROOT}/paddle/fluid/API_PR.spec fi if [ "${op_type_spec_diff}" != "" ] ; then echo "op_type_spec_diff: ${op_type_spec_diff}" diff --git a/tools/check_file_diff_approvals.sh b/tools/check_file_diff_approvals.sh index 92e59675dad..b43e2280294 100644 --- a/tools/check_file_diff_approvals.sh +++ b/tools/check_file_diff_approvals.sh @@ -54,6 +54,7 @@ API_FILES=("CMakeLists.txt" "python/paddle/fluid/tests/unittests/white_list/no_grad_set_white_list.py" "tools/print_signatures.py" "tools/sampcd_processor.py" + "tools/check_pr_approval.py" "paddle/scripts/paddle_build.bat" "tools/windows/run_unittests.sh" "tools/parallel_UT_rule.py" @@ -146,6 +147,9 @@ for API_FILE in ${API_FILES[*]}; do elif [ "${API_FILE}" == "tools/print_signatures.py" ];then echo_line="test_print_signatures.py will be executed for changed print_signatures.py.\n" run_tools_test test_print_signatures.py + elif [ "${API_FILE}" == "tools/checkout_pr_approval.py" ];then + echo_line="test_checkout_pr_approval.py will be executed for changed checkout_pr_approval.py.\n" + run_tools_test test_checkout_pr_approval.py elif [ "${API_FILE}" == "python/paddle/distributed/fleet/__init__.py" ]; then echo_line="You must have (fuyinno4 (Recommend), raindrops2sea) approval for ${API_FILE} changes" check_approval 1 35824027 38231817 diff --git a/tools/check_pr_approval.py b/tools/check_pr_approval.py index 937b0be7562..c242afd06e7 100644 --- a/tools/check_pr_approval.py +++ b/tools/check_pr_approval.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -from __future__ import print_function import sys import json @@ -24,17 +23,24 @@ def check_approval(count, required_reviewers): json_resp = json.loads(json_buff) approves = 0 approved_user_ids = [] + approved_user_logins = set() for review in json_resp: if review["state"] == "APPROVED": approves += 1 approved_user_ids.append(review["user"]["id"]) + approved_user_logins.add(review["user"]["login"]) # convert to int required_reviewers_int = set() + required_reviewers_login = set() for rr in required_reviewers: - required_reviewers_int.add(int(rr)) + if rr.isdigit(): + required_reviewers_int.add(int(rr)) + else: + required_reviewers_login.add(rr) - if len(set(approved_user_ids) & required_reviewers_int) >= count: + if len(set(approved_user_ids) & required_reviewers_int) + len( + approved_user_logins & required_reviewers_login) >= count: print("TRUE") else: print("FALSE") diff --git a/tools/print_signatures.py b/tools/print_signatures.py index b96ddcf549e..d4745b39711 100644 --- a/tools/print_signatures.py +++ b/tools/print_signatures.py @@ -17,20 +17,14 @@ Print all signature of a python module in alphabet order. Usage: ./print_signature "paddle.fluid" > signature.txt """ -from __future__ import print_function -import importlib import inspect import collections import sys -import pydoc import hashlib -import platform -import functools import pkgutil import logging import argparse -import paddle member_dict = collections.OrderedDict() @@ -80,11 +74,7 @@ def is_primitive(instance): ErrorSet = set() IdSet = set() -skiplist = [ - 'paddle.vision.datasets.DatasetFolderImageFolder', - 'paddle.truncdigamma', - 'paddle.fluid.layers.ops.func', -] +skiplist = [] def visit_all_module(mod): @@ -140,6 +130,7 @@ def get_all_api(root_path='paddle', attr="__all__"): """ walk through the paddle package to collect all the apis. """ + import paddle global api_info_dict api_counter = 0 for filefinder, name, ispkg in pkgutil.walk_packages( @@ -229,6 +220,7 @@ def process_module(m, attr="__all__"): def get_all_api_from_modulelist(): + import paddle modulelist = [paddle] for m in modulelist: visit_all_module(m) diff --git a/tools/sampcd_processor.py b/tools/sampcd_processor.py index 5acf9dc7d76..3ec12c11a70 100644 --- a/tools/sampcd_processor.py +++ b/tools/sampcd_processor.py @@ -27,7 +27,6 @@ import subprocess import multiprocessing import platform import inspect -import json import argparse import shutil import re diff --git a/tools/test_check_pr_approval.py b/tools/test_check_pr_approval.py new file mode 100644 index 00000000000..f4c089ee0f8 --- /dev/null +++ b/tools/test_check_pr_approval.py @@ -0,0 +1,120 @@ +#! /usr/bin/env python + +# Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +TestCases for check_pr_approval.py +""" +import unittest +import subprocess +import sys + + +class Test_check_approval(unittest.TestCase): + def setUp(self): + self.codeset = 'UTF-8' + # only key info in it + self.jsonstr = """ +[ + { + "id": 688077074, + "node_id": "MDE3OlB1bGxSZXF1ZXN0UmV2aWV3Njg4MDc3MDc0", + "user": { + "login": "wadefelix", + "id": 1306724, + "type": "User", + "site_admin": false + }, + "body": "", + "state": "COMMENTED", + "author_association": "CONTRIBUTOR" + }, + { + "id": 688092580, + "node_id": "MDE3OlB1bGxSZXF1ZXN0UmV2aWV3Njg4MDkyNTgw", + "user": { + "login": "MingMingShangTian", + "id": 13469016, + "type": "User", + "site_admin": false + }, + "body": "LGTM", + "state": "APPROVED", + "author_association": "CONTRIBUTOR" + }, + { + "id": 689175539, + "node_id": "MDE3OlB1bGxSZXF1ZXN0UmV2aWV3Njg5MTc1NTM5", + "user": { + "login": "pangyoki", + "id": 26408901, + "type": "User", + "site_admin": false + }, + "body": "LGTM", + "state": "APPROVED", + "author_association": "CONTRIBUTOR" + } +] +""".encode(self.codeset) + + def test_ids(self): + cmd = [sys.executable, 'check_pr_approval.py', '1', '26408901'] + subprc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + output, error = subprc.communicate(input=self.jsonstr) + self.assertEqual('TRUE', output.decode(self.codeset).rstrip()) + + def test_logins(self): + cmd = [sys.executable, 'check_pr_approval.py', '1', 'pangyoki'] + subprc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + output, error = subprc.communicate(input=self.jsonstr) + self.assertEqual('TRUE', output.decode(self.codeset).rstrip()) + + def test_ids_and_logins(self): + cmd = [ + sys.executable, 'check_pr_approval.py', '2', 'pangyoki', '13469016' + ] + subprc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + output, error = subprc.communicate(input=self.jsonstr) + #self.assertEqual('', error.rstrip()) + self.assertEqual('TRUE', output.decode(self.codeset).rstrip()) + + def test_check_with_required_reviewer_not_approved(self): + cmd = [ + sys.executable, 'check_pr_approval.py', '2', 'wadefelix', + ' 13469016' + ] + subprc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + output, error = subprc.communicate(input=self.jsonstr) + self.assertEqual('FALSE', output.decode(self.codeset).rstrip()) + + +if __name__ == '__main__': + unittest.main() diff --git a/tools/test_print_signatures.py b/tools/test_print_signatures.py index 7cbdbb56cb1..1ca1e4149fb 100644 --- a/tools/test_print_signatures.py +++ b/tools/test_print_signatures.py @@ -23,13 +23,9 @@ sample lines from API_DEV.spec: """ import unittest import hashlib -import inspect import functools from print_signatures import md5 -from print_signatures import get_functools_partial_spec -from print_signatures import format_spec -from print_signatures import queue_dict -from print_signatures import member_dict +from print_signatures import is_primitive def func_example(param_a, param_b): @@ -65,30 +61,27 @@ class Test_all_in_print_signatures(unittest.TestCase): digest = algo.hexdigest() self.assertEqual(digest, md5(func_example.__doc__)) - def test_get_functools_partial_spec(self): - partailed_func = functools.partial(func_example, 1) - # args = inspect.getargspec(partailed_func) - self.assertEqual('func_example(args=(1,), keywords={})', - get_functools_partial_spec(partailed_func)) - -class Test_format_spec(unittest.TestCase): - def test_normal_func_spec(self): - args = inspect.getargspec(func_example) - self.assertEqual( - '''ArgSpec(args=['param_a', 'param_b'], varargs=None, keywords=None, defaults=None)''', - format_spec(args)) - - def test_func_spec_with_partialedfunc_as_param_default(self): - # but there is no function belongs to this type in API_DEV.spec - args = inspect.getargspec(func_example_2) - self.assertEqual( - '''ArgSpec(args=['func'], varargs=None, keywords=None, defaults=('func_example(args=(1,), keywords={})',))''', - format_spec(args)) - - -class Test_queue_dict(unittest.TestCase): - pass +class Test_is_primitive(unittest.TestCase): + def test_single(self): + self.assertTrue(is_primitive(2)) + self.assertTrue(is_primitive(2.1)) + self.assertTrue(is_primitive("2.1.1")) + self.assertFalse( + is_primitive("hello paddle".encode('UTF-8'))) # True for python2 + self.assertFalse(is_primitive(1j)) + self.assertTrue(is_primitive(True)) + + def test_collection(self): + self.assertTrue(is_primitive([])) + self.assertTrue(is_primitive(tuple())) + self.assertTrue(is_primitive(set())) + self.assertTrue(is_primitive([1, 2])) + self.assertTrue(is_primitive((1.1, 2.2))) + self.assertTrue(is_primitive(set([1, 2.3]))) + self.assertFalse(is_primitive(range(3))) # True for python2 + self.assertFalse(is_primitive({})) + self.assertFalse(is_primitive([1, 1j])) if __name__ == '__main__': diff --git a/tools/test_sampcd_processor.py b/tools/test_sampcd_processor.py index 81710dae167..8963ae35f6b 100644 --- a/tools/test_sampcd_processor.py +++ b/tools/test_sampcd_processor.py @@ -16,10 +16,7 @@ import unittest import os -import tempfile import shutil -import sys -import importlib import re import sampcd_processor from sampcd_processor import find_all -- GitLab