gemini-code-assist[bot] commented on code in PR #309: URL: https://github.com/apache/tvm-ffi/pull/309#discussion_r2587260961
########## python/tvm_ffi/utils/kwargs_wrapper.py: ########## @@ -0,0 +1,309 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. +"""Utilities for creating high-performance keyword argument wrapper functions. + +This module provides tools for wrapping positional-only callables with +keyword argument support using code generation techniques. +""" + +from __future__ import annotations + +import inspect +from typing import Any, Callable + +# Sentinel object for missing arguments +MISSING = object() + + +def make_kwargs_wrapper( # noqa: PLR0912, PLR0915 + target_func: Callable, + args_names: list[str], + args_defaults: tuple = (), + kwargsonly_names: list[str] | None = None, + kwargsonly_defaults: dict[str, Any] | None = None, + prototype_func: Callable | None = None, +) -> Callable: + """Create a high-performance wrapper function with argument validation and default values. + + This function dynamically generates a wrapper using code generation to minimize overhead + while maintaining safety. The API is designed to align with Python's internal function + representation (__defaults__ and __kwdefaults__). + + The generated wrapper uses a sentinel object (MISSING) to distinguish between explicitly + passed arguments and those that should use default values. This avoids expensive inspection + and allows the generated code to be highly optimized by Python's bytecode compiler. + + Parameters + ---------- + target_func + The underlying function to be called by the wrapper only accepts positional arguments. + args_names + A list of ALL positional argument names in order. These define the positional + parameters that the wrapper will accept. Must not overlap with kwargsonly_names. + args_defaults + A tuple of default values for positional arguments, RIGHT-ALIGNED to args_names + (matching Python's __defaults__ behavior). The length of this tuple determines + how many trailing arguments have defaults. + Example: (10, 20) with args_names=['a', 'b', 'c', 'd'] means c=10, d=20. + Empty tuple () means no defaults. + kwargsonly_names + A list of keyword-only argument names. These arguments can only be passed by name, + not positionally, and appear after a '*' separator in the signature. Can include both + required and optional keyword-only arguments. Must not overlap with args_names. + Example: ['debug', 'timeout'] creates wrapper(..., *, debug, timeout). + kwargsonly_defaults + Optional dictionary of default values for keyword-only arguments (matching Python's + __kwdefaults__ behavior). Keys must be a subset of kwargsonly_names. Keyword-only + arguments not in this dict are required. + Example: {'timeout': 30} with kwargsonly_names=['debug', 'timeout'] means 'debug' + is required and 'timeout' is optional. + prototype_func + Optional prototype function to copy metadata (__name__, __doc__, __module__, + __qualname__, __annotations__) from. If None, no metadata is copied. + + Returns + ------- + A dynamically generated wrapper function with the specified signature + + """ + # We need to validate all arguments are valid Python identifiers, + # We also need to make sure that the default values are not supplied + # directly in generated code, instead we use a MISSING sentinel object to distinguish + # between explicitly passed arguments and those that should use default values + # then explicitly pass the defaults dictionary as exec_globals. + # Internal variable names used in generated code to avoid user argument conflicts + INTERNAL_TARGET_FUNC = "__i_target_func" + INTERNAL_MISSING = "__i_MISSING" + INTERNAL_DEFAULTS_DICT = "__i_args_defaults" + INTERNAL_NAMES = {INTERNAL_TARGET_FUNC, INTERNAL_MISSING, INTERNAL_DEFAULTS_DICT} + + if kwargsonly_names is None: + kwargsonly_names = [] + if kwargsonly_defaults is None: + kwargsonly_defaults = {} + + # Validate args_names are valid Python identifiers + for name in args_names: + if not isinstance(name, str): + raise TypeError(f"Argument name must be a string, got {type(name).__name__}: {name!r}") + if not name.isidentifier(): + raise ValueError(f"Invalid argument name: {name!r} is not a valid Python identifier") + + # Validate args_defaults is a tuple + if not isinstance(args_defaults, tuple): + raise TypeError(f"args_defaults must be a tuple, got {type(args_defaults).__name__}") + + # Validate args_defaults length doesn't exceed args_names length + if len(args_defaults) > len(args_names): + raise ValueError( + f"args_defaults has {len(args_defaults)} values but only " + f"{len(args_names)} positional arguments" + ) + + # Validate kwargsonly_names are valid identifiers + for name in kwargsonly_names: + if not isinstance(name, str): + raise TypeError( + f"Keyword-only argument name must be a string, got {type(name).__name__}: {name!r}" + ) + if not name.isidentifier(): + raise ValueError( + f"Invalid keyword-only argument name: {name!r} is not a valid Python identifier" + ) + + # Validate kwargsonly_defaults keys are in kwargsonly_names + kwargsonly_names_set = set(kwargsonly_names) + for key in kwargsonly_defaults: + if key not in kwargsonly_names_set: + raise ValueError( + f"Default provided for '{key}' which is not in kwargsonly_names: {kwargsonly_names}" + ) + + # Validate no overlap between positional and keyword-only arguments + args_names_set = set(args_names) + overlap = args_names_set & kwargsonly_names_set + if overlap: + raise ValueError(f"Arguments cannot be both positional and keyword-only: {overlap}") + + # Validate no conflict between user argument names and internal names + all_user_names = args_names_set | kwargsonly_names_set + conflicts = all_user_names & INTERNAL_NAMES + if conflicts: + raise ValueError( + f"Argument names conflict with internal names: {conflicts}. " + f'Please avoid using names starting with "__i_"' + ) + + # Convert args_defaults tuple to dict for efficient lookup (right-aligned) + # Example: args_names=["a","b","c","d"], args_defaults=(10,20) -> {"c":10, "d":20} + num_defaults = len(args_defaults) + args_defaults_dict = {} + if num_defaults > 0: + for i, default_value in enumerate(args_defaults): + param_index = len(args_names) - num_defaults + i + args_defaults_dict[args_names[param_index]] = default_value + + # Combine all defaults into a single dict for efficiency + all_defaults = {**args_defaults_dict, **kwargsonly_defaults} + + # Build wrapper signature and call arguments + arg_parts = [] + call_parts = [] + + # Handle positional arguments + for name in args_names: + if name in args_defaults_dict: + # Use the sentinel in the signature. + arg_parts.append(f"{name}={INTERNAL_MISSING}") + # The conditional check runs. + call_parts.append( + f'{INTERNAL_DEFAULTS_DICT}["{name}"] if {name} is {INTERNAL_MISSING} else {name}' + ) + else: + arg_parts.append(name) + call_parts.append(name) + + # Handle keyword-only arguments + if kwargsonly_names: + arg_parts.append("*") # Separator for keyword-only args + for name in kwargsonly_names: + if name in kwargsonly_defaults: + # Optional keyword-only arg (has default) + arg_parts.append(f"{name}={INTERNAL_MISSING}") + call_parts.append( + f'{INTERNAL_DEFAULTS_DICT}["{name}"] if {name} is {INTERNAL_MISSING} else {name}' + ) + else: + # Required keyword-only arg (no default) + arg_parts.append(name) + call_parts.append(name) + + arg_list = ", ".join(arg_parts) + call_list = ", ".join(call_parts) + + code_str = f""" +def wrapper({arg_list}): + return {INTERNAL_TARGET_FUNC}({call_list}) +""" + exec_globals = { + INTERNAL_TARGET_FUNC: target_func, + INTERNAL_MISSING: MISSING, + INTERNAL_DEFAULTS_DICT: all_defaults, + } + namespace: dict[str, Any] = {} + # Note: this is a limited use of exec that is safe. + # We ensure generated code does not contain any untrusted input. + # the argument names are validated and the default values are not part of generated code. + # instead default values are set to MISSING sentinel object and explicitly passed as exec_globals. + # This is a practice adopted by `dataclasses` and `pydantic` + exec(code_str, exec_globals, namespace) + # Retrieve the function object and return it + new_func = namespace["wrapper"] + + # Copy metadata from prototype_func if provided + if prototype_func is not None: + if hasattr(prototype_func, "__name__"): + new_func.__name__ = prototype_func.__name__ + if hasattr(prototype_func, "__doc__"): + new_func.__doc__ = prototype_func.__doc__ + if hasattr(prototype_func, "__module__"): + new_func.__module__ = prototype_func.__module__ + if hasattr(prototype_func, "__qualname__"): + new_func.__qualname__ = prototype_func.__qualname__ + if hasattr(prototype_func, "__annotations__"): + new_func.__annotations__ = prototype_func.__annotations__.copy() + + return new_func Review Comment:  The function `make_kwargs_wrapper` is quite long and complex, as indicated by the `noqa` for `PLR0912` (too many branches) and `PLR0915` (too many statements). To improve maintainability and readability, consider refactoring it into smaller, more focused helper functions. For example: * A function for input validation (e.g., `_validate_wrapper_args`). Much of the validation logic is duplicated for `args_names` and `kwargsonly_names` and could be consolidated. * A function for preparing the components of the generated code (e.g., `_build_wrapper_components`). The main function would then orchestrate calls to these helpers. This would make the logic of each part clearer and the main function easier to follow. ########## python/tvm_ffi/utils/kwargs_wrapper.py: ########## @@ -0,0 +1,309 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. +"""Utilities for creating high-performance keyword argument wrapper functions. + +This module provides tools for wrapping positional-only callables with +keyword argument support using code generation techniques. +""" + +from __future__ import annotations + +import inspect +from typing import Any, Callable + +# Sentinel object for missing arguments +MISSING = object() + + +def make_kwargs_wrapper( # noqa: PLR0912, PLR0915 + target_func: Callable, + args_names: list[str], + args_defaults: tuple = (), + kwargsonly_names: list[str] | None = None, + kwargsonly_defaults: dict[str, Any] | None = None, + prototype_func: Callable | None = None, +) -> Callable: + """Create a high-performance wrapper function with argument validation and default values. + + This function dynamically generates a wrapper using code generation to minimize overhead + while maintaining safety. The API is designed to align with Python's internal function + representation (__defaults__ and __kwdefaults__). + + The generated wrapper uses a sentinel object (MISSING) to distinguish between explicitly + passed arguments and those that should use default values. This avoids expensive inspection + and allows the generated code to be highly optimized by Python's bytecode compiler. + + Parameters + ---------- + target_func + The underlying function to be called by the wrapper only accepts positional arguments. + args_names + A list of ALL positional argument names in order. These define the positional + parameters that the wrapper will accept. Must not overlap with kwargsonly_names. + args_defaults + A tuple of default values for positional arguments, RIGHT-ALIGNED to args_names + (matching Python's __defaults__ behavior). The length of this tuple determines + how many trailing arguments have defaults. + Example: (10, 20) with args_names=['a', 'b', 'c', 'd'] means c=10, d=20. + Empty tuple () means no defaults. + kwargsonly_names + A list of keyword-only argument names. These arguments can only be passed by name, + not positionally, and appear after a '*' separator in the signature. Can include both + required and optional keyword-only arguments. Must not overlap with args_names. + Example: ['debug', 'timeout'] creates wrapper(..., *, debug, timeout). + kwargsonly_defaults + Optional dictionary of default values for keyword-only arguments (matching Python's + __kwdefaults__ behavior). Keys must be a subset of kwargsonly_names. Keyword-only + arguments not in this dict are required. + Example: {'timeout': 30} with kwargsonly_names=['debug', 'timeout'] means 'debug' + is required and 'timeout' is optional. + prototype_func + Optional prototype function to copy metadata (__name__, __doc__, __module__, + __qualname__, __annotations__) from. If None, no metadata is copied. + + Returns + ------- + A dynamically generated wrapper function with the specified signature + + """ + # We need to validate all arguments are valid Python identifiers, + # We also need to make sure that the default values are not supplied + # directly in generated code, instead we use a MISSING sentinel object to distinguish + # between explicitly passed arguments and those that should use default values + # then explicitly pass the defaults dictionary as exec_globals. + # Internal variable names used in generated code to avoid user argument conflicts + INTERNAL_TARGET_FUNC = "__i_target_func" + INTERNAL_MISSING = "__i_MISSING" + INTERNAL_DEFAULTS_DICT = "__i_args_defaults" + INTERNAL_NAMES = {INTERNAL_TARGET_FUNC, INTERNAL_MISSING, INTERNAL_DEFAULTS_DICT} + + if kwargsonly_names is None: + kwargsonly_names = [] + if kwargsonly_defaults is None: + kwargsonly_defaults = {} + + # Validate args_names are valid Python identifiers + for name in args_names: + if not isinstance(name, str): + raise TypeError(f"Argument name must be a string, got {type(name).__name__}: {name!r}") + if not name.isidentifier(): + raise ValueError(f"Invalid argument name: {name!r} is not a valid Python identifier") + + # Validate args_defaults is a tuple + if not isinstance(args_defaults, tuple): + raise TypeError(f"args_defaults must be a tuple, got {type(args_defaults).__name__}") + + # Validate args_defaults length doesn't exceed args_names length + if len(args_defaults) > len(args_names): + raise ValueError( + f"args_defaults has {len(args_defaults)} values but only " + f"{len(args_names)} positional arguments" + ) + + # Validate kwargsonly_names are valid identifiers + for name in kwargsonly_names: + if not isinstance(name, str): + raise TypeError( + f"Keyword-only argument name must be a string, got {type(name).__name__}: {name!r}" + ) + if not name.isidentifier(): + raise ValueError( + f"Invalid keyword-only argument name: {name!r} is not a valid Python identifier" + ) + + # Validate kwargsonly_defaults keys are in kwargsonly_names + kwargsonly_names_set = set(kwargsonly_names) + for key in kwargsonly_defaults: + if key not in kwargsonly_names_set: + raise ValueError( + f"Default provided for '{key}' which is not in kwargsonly_names: {kwargsonly_names}" + ) + + # Validate no overlap between positional and keyword-only arguments + args_names_set = set(args_names) + overlap = args_names_set & kwargsonly_names_set + if overlap: + raise ValueError(f"Arguments cannot be both positional and keyword-only: {overlap}") + + # Validate no conflict between user argument names and internal names + all_user_names = args_names_set | kwargsonly_names_set + conflicts = all_user_names & INTERNAL_NAMES + if conflicts: + raise ValueError( + f"Argument names conflict with internal names: {conflicts}. " + f'Please avoid using names starting with "__i_"' + ) + + # Convert args_defaults tuple to dict for efficient lookup (right-aligned) + # Example: args_names=["a","b","c","d"], args_defaults=(10,20) -> {"c":10, "d":20} + num_defaults = len(args_defaults) + args_defaults_dict = {} + if num_defaults > 0: + for i, default_value in enumerate(args_defaults): + param_index = len(args_names) - num_defaults + i + args_defaults_dict[args_names[param_index]] = default_value Review Comment:  This logic for creating `args_defaults_dict` can be expressed more concisely and Pythonically using `zip` with slicing. This improves readability and is a more common pattern for this task. ```suggestion args_defaults_dict = dict(zip(args_names[-len(args_defaults) :], args_defaults)) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
