From 1889d504c413d73a6f025b61d21b4d95198e9d3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=20Wei=20=28=E4=BB=BB=E5=8D=AB=29?= Date: Thu, 15 Jul 2021 14:28:23 +0800 Subject: [PATCH] Check api compatible (#34119) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * print the signatures of the apis * some faults, and a test case * use the FullArgSpec as the ArgSpec, repr>str 为了延续脚本历史，把FullArgsSpec替换为ArgSpec；在逆转回对象时，记得加上inpect.FullArgSpec对象 * script form @zhiboniu * parsearg * read argspec from file * add comments * refactor check_compatible and its testcases * skip the empty ArgSepc() * update the function, get inspect.FullArgSpec instances directly. * typo * for the functions using inner-defined VarType or other Class Type not accessable, use the string comparements. * typo * logging added * 若变量和默认值都增加了，用负数索引需要再处理下这个增加的内容 * defaults itself may be NoneType * defaults added 2 items when args only added 1 item * same as the previous comment * enable the check_api_compatible.py * run test_check_api_compatible.py * sort the output of print_signatures.py --- tools/check_api_approvals.sh | 7 +- tools/check_api_compatible.py | 171 +++++++++++++++++++++++++++++ tools/check_file_diff_approvals.sh | 3 + tools/print_signatures.py | 30 +++-- tools/test_check_api_compatible.py | 141 ++++++++++++++++++++++++ tools/test_sampcd_processor.py | 3 + 6 files changed, 347 insertions(+), 8 deletions(-) create mode 100644 tools/check_api_compatible.py create mode 100644 tools/test_check_api_compatible.py diff --git a/tools/check_api_approvals.sh b/tools/check_api_approvals.sh index 40a0a618fb0..c0a0b754d51 100644 --- a/tools/check_api_approvals.sh +++ b/tools/check_api_approvals.sh @@ -37,8 +37,9 @@ function add_failed(){ } +api_params_diff=`python ${PADDLE_ROOT}/tools/check_api_compatible.py ${PADDLE_ROOT}/paddle/fluid/API_DEV.spec ${PADDLE_ROOT}/paddle/fluid/API_PR.spec` 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` -if [ "$api_spec_diff" != "" ]; then +if [ "$api_spec_diff" != "" -o "${api_params_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" @@ -87,6 +88,7 @@ if [ "${ADDED_OP_USE_DEFAULT_GRAD_MAKER}" != "" ]; then check_approval 1 6888866 7913861 fi + if [ -n "${echo_list}" ];then echo "****************" echo -e "${echo_list[@]}" @@ -97,6 +99,9 @@ if [ -n "${echo_list}" ];then 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 [ "${api_params_diff}" != "" ] ; then + echo "api_params_diff: ${api_params_diff}" + fi if [ "${op_type_spec_diff}" != "" ] ; then echo "op_type_spec_diff: ${op_type_spec_diff}" fi diff --git a/tools/check_api_compatible.py b/tools/check_api_compatible.py new file mode 100644 index 00000000000..cc307a88466 --- /dev/null +++ b/tools/check_api_compatible.py @@ -0,0 +1,171 @@ +# 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. + +import argparse +import inspect +import sys +import re +import logging + +logger = logging.getLogger() +if logger.handlers: + # we assume the first handler is the one we want to configure + console = logger.handlers[0] +else: + console = logging.StreamHandler(sys.stderr) + logger.addHandler(console) +console.setFormatter( + logging.Formatter( + "%(asctime)s - %(funcName)s:%(lineno)d - %(levelname)s - %(message)s")) + + +def _check_compatible(args_o, args_n, defaults_o, defaults_n): + # 如果参数减少了，需要提醒关注 + if len(args_o) > len(args_n): + logger.debug("args num less then previous: %s vs %s", args_o, args_n) + return False + # 参数改名了，也要提醒关注 + for idx in range(min(len(args_o), len(args_n))): + if args_o[idx] != args_n[idx]: + logger.debug("args's %d parameter diff with previous: %s vs %s", + idx, args_o, args_n) + return False + # 新增加了参数，必须提供默认值。以及不能减少默认值数量 + if (len(args_n) - len(defaults_n)) > (len(args_o) - len(defaults_o)): + logger.debug("defaults num less then previous: %s vs %s", defaults_o, + defaults_n) + return False + # 默认值必须相等 + for idx in range(min(len(defaults_o), len(defaults_n))): + nidx_o = -1 - idx + nidx_n = -1 - idx - (len(args_n) - len(args_o)) + if (defaults_o[nidx_o] != defaults_n[nidx_n]): + logger.debug("defaults's %d value diff with previous: %s vs %s", + nidx_n, defaults_o, defaults_n) + return False + return True + + +def check_compatible(old_api_spec, new_api_spec): + """ + check compatible, FullArgSpec + """ + if not (isinstance(old_api_spec, inspect.FullArgSpec) and isinstance( + new_api_spec, inspect.FullArgSpec)): + logger.warning( + "new_api_spec or old_api_spec is not instance of inspect.FullArgSpec" + ) + return False + return _check_compatible( + old_api_spec.args, new_api_spec.args, [] + if old_api_spec.defaults is None else old_api_spec.defaults, [] + if new_api_spec.defaults is None else new_api_spec.defaults) + + +def check_compatible_str(old_api_spec_str, new_api_spec_str): + patArgSpec = re.compile( + r'args=(.*), varargs=.*defaults=\((.*)\), kwonlyargs=.*') + mo_o = patArgSpec.search(old_api_spec_str) + mo_n = patArgSpec.search(new_api_spec_str) + if not (mo_o and mo_n): + # error + logger.warning("old_api_spec_str: %s", old_api_spec_str) + logger.warning("new_api_spec_str: %s", new_api_spec_str) + return False + + args_o = eval(mo_o.group(1)) + args_n = eval(mo_n.group(1)) + defaults_o = mo_o.group(2).split(', ') + defaults_n = mo_n.group(2).split(', ') + return _check_compatible(args_o, args_n, defaults_o, defaults_n) + + +def read_argspec_from_file(specfile): + """ + read FullArgSpec from spec file + """ + res_dict = {} + patArgSpec = re.compile( + r'^(paddle[^,]+)\s+\((ArgSpec.*),\s\(\'document\W*([0-9a-z]{32})') + fullargspec_prefix = 'inspect.Full' + for line in specfile.readlines(): + mo = patArgSpec.search(line) + if mo and mo.group(2) != 'ArgSpec()': + logger.debug("%s argspec: %s", mo.group(1), mo.group(2)) + try: + res_dict[mo.group(1)] = eval(fullargspec_prefix + mo.group(2)) + except: # SyntaxError, NameError: + res_dict[mo.group(1)] = fullargspec_prefix + mo.group(2) + return res_dict + + +arguments = [ + # flags, dest, type, default, help +] + + +def parse_args(): + """ + Parse input arguments + """ + global arguments + parser = argparse.ArgumentParser( + description='check api compatible across versions') + parser.add_argument('--debug', dest='debug', action="store_true") + parser.add_argument( + 'prev', + type=argparse.FileType('r'), + help='the previous version (the version from develop branch)') + parser.add_argument( + 'post', + type=argparse.FileType('r'), + help='the post version (the version from PullRequest)') + for item in arguments: + parser.add_argument( + item[0], dest=item[1], help=item[4], type=item[2], default=item[3]) + + if len(sys.argv) < 2: + parser.print_help() + sys.exit(1) + + args = parser.parse_args() + return args + + +if __name__ == '__main__': + args = parse_args() + if args.debug: + logger.setLevel(logging.DEBUG) + else: + logger.setLevel(logging.INFO) + if args.prev and args.post: + prev_spec = read_argspec_from_file(args.prev) + post_spec = read_argspec_from_file(args.post) + diff_api_names = [] + for as_post_name, as_post in post_spec.items(): + as_prev = prev_spec.get(as_post_name) + if as_prev is None: # the api is deleted + continue + if isinstance(as_prev, str) or isinstance(as_post, str): + as_prev_str = as_prev if isinstance(as_prev, + str) else repr(as_prev) + as_post_str = as_post if isinstance(as_post, + str) else repr(as_post) + if not check_compatible_str(as_prev_str, as_post_str): + diff_api_names.append(as_post_name) + else: + if not check_compatible(as_prev, as_post): + diff_api_names.append(as_post_name) + if diff_api_names: + print('\n'.join(diff_api_names)) diff --git a/tools/check_file_diff_approvals.sh b/tools/check_file_diff_approvals.sh index b43e2280294..7324b7a6c13 100644 --- a/tools/check_file_diff_approvals.sh +++ b/tools/check_file_diff_approvals.sh @@ -150,6 +150,9 @@ for API_FILE in ${API_FILES[*]}; do 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}" == "tools/checkout_api_compatible.py" ];then + echo_line="test_checkout_api_compatible.py will be executed for changed checkout_api_compatible.py.\n" + run_tools_test test_checkout_api_compatible.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/print_signatures.py b/tools/print_signatures.py index 65e7c7e0efc..b9be7f836a4 100644 --- a/tools/print_signatures.py +++ b/tools/print_signatures.py @@ -160,14 +160,16 @@ def insert_api_into_dict(full_name, gen_doc_anno=None): Return: api_info object or None """ + import paddle try: obj = eval(full_name) fc_id = id(obj) except AttributeError: logger.warning("AttributeError occurred when `id(eval(%s))`", full_name) return None - except: - logger.warning("Exception occurred when `id(eval(%s))`", full_name) + except Exception as e: + logger.warning("Exception(%s) occurred when `id(eval(%s))`", + str(e), full_name) return None else: logger.debug("adding %s to api_info_dict.", full_name) @@ -186,6 +188,10 @@ def insert_api_into_dict(full_name, gen_doc_anno=None): api_info_dict[fc_id]["docstring"] = inspect.cleandoc(docstr) if gen_doc_anno: api_info_dict[fc_id]["gen_doc_anno"] = gen_doc_anno + if inspect.isfunction(obj): + api_info_dict[fc_id]["signature"] = repr( + inspect.getfullargspec(obj)).replace('FullArgSpec', + 'ArgSpec', 1) return api_info_dict[fc_id] @@ -311,7 +317,7 @@ def parse_args(): '--method', dest='method', type=str, - default='from_modulelist', + default='get_all_api', help="using get_all_api or from_modulelist") parser.add_argument( 'module', type=str, help='module', default='paddle') # not used @@ -334,10 +340,20 @@ if __name__ == '__main__': for name in member_dict: print(name, member_dict[name]) elif args.method == 'get_all_api': - api_signs = get_all_api() - for api_sign in api_signs: - print("{0} ({0}, ('document', '{1}'))".format(api_sign[0], api_sign[ - 1])) + get_all_api() + all_api_names_to_k = {} + for k, api_info in api_info_dict.items(): + # 1. the shortest suggested_name may be renamed; + # 2. some api's fullname is not accessable, the module name of it is overrided by the function with the same name; + api_name = sorted(list(api_info['all_names']))[0] + all_api_names_to_k[api_name] = k + all_api_names_sorted = sorted(all_api_names_to_k.keys()) + for api_name in all_api_names_sorted: + api_info = api_info_dict[all_api_names_to_k[api_name]] + print("{0} ({2}, ('document', '{1}'))".format( + api_name, + md5(api_info['docstring']), api_info['signature'] + if 'signature' in api_info else 'ArgSpec()')) if len(ErrorSet) == 0: sys.exit(0) diff --git a/tools/test_check_api_compatible.py b/tools/test_check_api_compatible.py new file mode 100644 index 00000000000..23debef657f --- /dev/null +++ b/tools/test_check_api_compatible.py @@ -0,0 +1,141 @@ +#! /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_api_compatible.py +""" +import unittest +import sys +import os +import tempfile +import inspect + +from check_api_compatible import read_argspec_from_file +from check_api_compatible import check_compatible +from check_api_compatible import check_compatible_str + + +class Test_check_compatible(unittest.TestCase): + def setUp(self) -> None: + self.fullargspec_prefix = 'inspect.Full' + self.argspec_str_o = self.fullargspec_prefix + '''ArgSpec(args=['shape', 'dtype', 'name'], varargs=None, varkw=None, defaults=(None, None), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + return super().setUp() + + def test_normal_not_changed(self): + argspec_o = eval(self.argspec_str_o) + argspec_n = eval(self.argspec_str_o) + self.assertTrue(check_compatible(argspec_o, argspec_n)) + + def test_args_added(self): + argspec_str_n = '''ArgSpec(args=['shape', 'dtype', 'name', 'arg4'], varargs=None, varkw=None, defaults=(None, None), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + argspec_o = eval(self.argspec_str_o) + argspec_n = eval(self.fullargspec_prefix + argspec_str_n) + self.assertFalse(check_compatible(argspec_o, argspec_n)) + + argspec_str_n = '''ArgSpec(args=['shape', 'dtype', 'name', 'arg4'], varargs=None, varkw=None, defaults=(None, None, 1), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + argspec_n = eval(self.fullargspec_prefix + argspec_str_n) + self.assertTrue(check_compatible(argspec_o, argspec_n)) + + argspec_str_n = '''ArgSpec(args=['shape', 'dtype', 'name', 'arg4'], varargs=None, varkw=None, defaults=(None, None, 1, True), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + argspec_n = eval(self.fullargspec_prefix + argspec_str_n) + self.assertFalse(check_compatible(argspec_o, argspec_n)) + + argspec_str_n = '''ArgSpec(args=['shape', 'dtype', 'name', 'arg4'], varargs=None, varkw=None, defaults=(True, None, None, 1), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + argspec_n = eval(self.fullargspec_prefix + argspec_str_n) + self.assertTrue(check_compatible(argspec_o, argspec_n)) + + def test_args_places_exchanged(self): + argspec_str_n = '''ArgSpec(args=['shape', 'name', 'dtype'], varargs=None, varkw=None, defaults=(None, None), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + argspec_o = eval(self.argspec_str_o) + argspec_n = eval(self.fullargspec_prefix + argspec_str_n) + self.assertFalse(check_compatible(argspec_o, argspec_n)) + + def test_args_reduced(self): + argspec_str_n = '''ArgSpec(args=['shape', 'name'], varargs=None, varkw=None, defaults=(None,), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + argspec_o = eval(self.argspec_str_o) + argspec_n = eval(self.fullargspec_prefix + argspec_str_n) + self.assertFalse(check_compatible(argspec_o, argspec_n)) + + +class Test_check_compatible_str(unittest.TestCase): + def setUp(self) -> None: + self.fullargspec_prefix = 'inspect.Full' + # paddle.fluid.layer_helper_base.LayerHelperBase.create_parameter + self.argspec_str_o = self.fullargspec_prefix + """ArgSpec(args=['self', 'attr', 'shape', 'dtype', 'is_bias', 'default_initializer', 'stop_gradient', 'type'], varargs=None, varkw=None, defaults=(None, False, None, False, VarType.LOD_TENSOR), kwonlyargs=[], kwonlydefaults=None, annotations={})""" + return super().setUp() + + def test_normal_not_changed(self): + argspec_o = self.argspec_str_o + argspec_n = self.argspec_str_o + self.assertTrue(check_compatible_str(argspec_o, argspec_n)) + + def test_args_added(self): + argspec_n = self.fullargspec_prefix + """ArgSpec(args=['self', 'attr', 'shape', 'dtype', 'is_bias', 'default_initializer', 'stop_gradient', 'type', 'argadded'], varargs=None, varkw=None, defaults=(None, False, None, False, VarType.LOD_TENSOR), kwonlyargs=[], kwonlydefaults=None, annotations={})""" + argspec_o = self.argspec_str_o + self.assertFalse(check_compatible_str(argspec_o, argspec_n)) + + argspec_n = self.fullargspec_prefix + """ArgSpec(args=['self', 'attr', 'shape', 'dtype', 'is_bias', 'default_initializer', 'stop_gradient', 'type', 'argadded'], varargs=None, varkw=None, defaults=(None, False, None, False, VarType.LOD_TENSOR, argadded), kwonlyargs=[], kwonlydefaults=None, annotations={})""" + self.assertTrue(check_compatible_str(argspec_o, argspec_n)) + + argspec_n = self.fullargspec_prefix + """ArgSpec(args=['self', 'attr', 'shape', 'dtype', 'is_bias', 'default_initializer', 'stop_gradient', 'type', 'argadded'], varargs=None, varkw=None, defaults=(None, False, None, False, VarType.LOD_TENSOR, argadded, 1), kwonlyargs=[], kwonlydefaults=None, annotations={})""" + self.assertFalse(check_compatible_str(argspec_o, argspec_n)) + + argspec_n = self.fullargspec_prefix + """ArgSpec(args=['self', 'attr', 'shape', 'dtype', 'is_bias', 'default_initializer', 'stop_gradient', 'type', 'argadded'], varargs=None, varkw=None, defaults=(1, None, False, None, False, VarType.LOD_TENSOR, argadded), kwonlyargs=[], kwonlydefaults=None, annotations={})""" + self.assertTrue(check_compatible_str(argspec_o, argspec_n)) + + def test_args_places_exchanged(self): + argspec_n = self.fullargspec_prefix + """ArgSpec(args=['self', 'attr', 'shape', 'dtype', 'is_bias', 'default_initializer', 'type', 'stop_gradient'], varargs=None, varkw=None, defaults=(None, False, None, False, VarType.LOD_TENSOR), kwonlyargs=[], kwonlydefaults=None, annotations={})""" + argspec_o = self.argspec_str_o + self.assertFalse(check_compatible_str(argspec_o, argspec_n)) + + def test_args_reduced(self): + argspec_n = self.fullargspec_prefix + """ArgSpec(args=['self', 'attr', 'shape', 'dtype', 'is_bias', 'default_initializer', 'stop_gradient'], varargs=None, varkw=None, defaults=(None, False, None, False, VarType.LOD_TENSOR), kwonlyargs=[], kwonlydefaults=None, annotations={})""" + argspec_o = self.argspec_str_o + self.assertFalse(check_compatible_str(argspec_o, argspec_n)) + + +class Test_read_argspec_from_file(unittest.TestCase): + def setUp(self) -> None: + self.fullargspec_prefix = 'inspect.Full' + self.argspec_str_o = self.fullargspec_prefix + '''ArgSpec(args=['shape', 'dtype', 'name'], varargs=None, varkw=None, defaults=(None, None), kwonlyargs=[], kwonlydefaults=None, annotations={})''' + self.api_spec_file = tempfile.TemporaryFile('w+t') + if self.api_spec_file: + self.api_spec_file.write("\n".join([ + """paddle.ones (ArgSpec(args=['shape', 'dtype', 'name'], varargs=None, varkw=None, defaults=(None, None), kwonlyargs=[], kwonlydefaults=None, annotations={}), ('document', '50a3b3a77fa13bb2ae4337d8f9d091b7'))""", + # """paddle.four_plus_four (paddle.four_plus_four, ('document', 'ff0f188c95030158cc6398d2a6c5four'))""", + """paddle.five_plus_five (ArgSpec(), ('document', 'ff0f188c95030158cc6398d2a6c5five'))""", + ])) + self.api_spec_file.seek(0) + return super().setUp() + + def tearDown(self): + if self.api_spec_file: + self.api_spec_file.close() + + def test_case_normal(self): + if self.api_spec_file: + api_argspec_dict = read_argspec_from_file(self.api_spec_file) + argspec = eval(self.argspec_str_o) + self.assertEqual( + api_argspec_dict.get('paddle.ones').args, argspec.args) + self.assertEqual( + api_argspec_dict.get('paddle.ones').defaults, argspec.defaults) + self.assertIsNone(api_argspec_dict.get('paddle.five_plus_five')) + else: + self.fail('api_spec_file error') + + +if __name__ == '__main__': + unittest.main() diff --git a/tools/test_sampcd_processor.py b/tools/test_sampcd_processor.py index 8963ae35f6b..2bcee0d2ae0 100644 --- a/tools/test_sampcd_processor.py +++ b/tools/test_sampcd_processor.py @@ -433,6 +433,7 @@ class Test_get_api_md5(unittest.TestCase): """paddle.two_plus_two (ArgSpec(args=[], varargs=None, keywords=None, defaults=(,)), ('document', 'ff0f188c95030158cc6398d2a6c55two'))""", """paddle.three_plus_three (ArgSpec(args=[], varargs=None, keywords=None, defaults=(,)), ('document', 'ff0f188c95030158cc6398d2a6cthree'))""", """paddle.four_plus_four (paddle.four_plus_four, ('document', 'ff0f188c95030158cc6398d2a6c5four'))""", + """paddle.five_plus_five (ArgSpec(), ('document', 'ff0f188c95030158cc6398d2a6c5five'))""", ])) def tearDown(self): @@ -449,6 +450,8 @@ class Test_get_api_md5(unittest.TestCase): res['paddle.three_plus_three']) self.assertEqual("ff0f188c95030158cc6398d2a6c5four", res['paddle.four_plus_four']) + self.assertEqual("ff0f188c95030158cc6398d2a6c5five", + res['paddle.five_plus_five']) class Test_get_incrementapi(unittest.TestCase): -- GitLab