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

@@ -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}"

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

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:
+        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")

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)

tools/sampcd_processor.py

@@ -27,7 +27,6 @@ import subprocess
 import multiprocessing
 import platform
 import inspect
-import json
 import argparse
 import shutil
 import re

tools/test_check_pr_approval.py

+#! /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()

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__':

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