gemini-code-assist[bot] commented on code in PR #309: URL: https://github.com/apache/tvm-ffi/pull/309#discussion_r2587275434
########## tests/scripts/benchmark_kwargs_wrapper.py: ########## @@ -0,0 +1,93 @@ +# 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. +"""Benchmark API overhead of kwargs wrapper. + +This benchmark compares the overhead of using make_kwargs_wrapper to add +keyword argument support to a positional-only function. We benchmark a +function that accepts three torch tensors where the last two have defaults. +""" + +from __future__ import annotations +import time + + +from tvm_ffi.utils.kwargs_wrapper import make_kwargs_wrapper + + +def print_speed(name: str, speed: float) -> None: + print(f"{name:<60} {speed} sec/call") + + +def target_func(*args): + pass + + +def benchmark_kwargs_wrapper(repeat: int = 1000000) -> None: Review Comment:  For benchmarking short durations, `time.perf_counter()` is more suitable than `time.time()`. `time.time()` measures wall-clock time and can be affected by system time adjustments, whereas `perf_counter()` provides a high-resolution monotonic clock specifically for measuring performance. For more accurate and reliable benchmark results, please replace all instances of `time.time()` with `time.perf_counter()` within this function. ########## python/tvm_ffi/utils/kwargs_wrapper.py: ########## @@ -0,0 +1,373 @@ +# 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() + +# 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} + + +def _validate_argument_names(names: list[str], arg_type: str) -> None: + """Validate that argument names are valid Python identifiers. + + Parameters + ---------- + names + List of argument names to validate. + arg_type + Description of the argument type (e.g., "Argument", "Keyword-only argument"). + + """ + for name in names: + if not isinstance(name, str): + raise TypeError( + f"{arg_type} name must be a string, got {type(name).__name__}: {name!r}" + ) + if not name.isidentifier(): + raise ValueError( + f"Invalid {arg_type.lower()} name: {name!r} is not a valid Python identifier" + ) + + +def _validate_wrapper_args( + args_names: list[str], + args_defaults: tuple, + kwargsonly_names: list[str], + kwargsonly_defaults: dict[str, Any], +) -> None: + """Validate all input arguments for make_kwargs_wrapper. + + Parameters + ---------- + args_names + List of positional argument names. + args_defaults + Tuple of default values for positional arguments. + kwargsonly_names + List of keyword-only argument names. + kwargsonly_defaults + Dictionary of default values for keyword-only arguments. + + """ + # Validate args_names are valid Python identifiers + _validate_argument_names(args_names, "Argument") + + # 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 + _validate_argument_names(kwargsonly_names, "Keyword-only argument") + + # 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_"' + ) + + +def _build_wrapper_code( + args_names: list[str], + kwargsonly_names: list[str], + all_defaults: dict[str, Any], +) -> str: + """Build the wrapper function code string. + + Parameters + ---------- + args_names + List of positional argument names. + kwargsonly_names + List of keyword-only argument names. + all_defaults + Dictionary of all default values. + + Returns + ------- + Generated Python code string for the wrapper function. + + """ + arg_parts = [] + call_parts = [] + + # Handle positional arguments + for name in args_names: + if name in all_defaults: + # 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 all_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) + + return f""" +def wrapper({arg_list}): + return {_INTERNAL_TARGET_FUNC}({call_list}) +""" + + +def _copy_function_metadata(target_func: Callable, source_func: Callable) -> None: + """Copy metadata from source function to target function. + + Parameters + ---------- + target_func + Function to copy metadata to. + source_func + Function to copy metadata from. + + """ + if hasattr(source_func, "__name__"): + target_func.__name__ = source_func.__name__ + if hasattr(source_func, "__doc__"): + target_func.__doc__ = source_func.__doc__ + if hasattr(source_func, "__module__"): + target_func.__module__ = source_func.__module__ + if hasattr(source_func, "__qualname__"): + target_func.__qualname__ = source_func.__qualname__ + if hasattr(source_func, "__annotations__"): + target_func.__annotations__ = source_func.__annotations__.copy() Review Comment:  The `_copy_function_metadata` function is a manual implementation of functionality provided by Python's standard library. For better maintainability, robustness, and adherence to standard practices, it's recommended to use `functools.update_wrapper` instead. The standard library function is more comprehensive, for instance, it also copies the `__dict__` attribute. You can refactor this by: 1. Adding `import functools` at the top of the file. 2. Removing the `_copy_function_metadata` function entirely. 3. Replacing the call at line 292 with `functools.update_wrapper(new_func, prototype_func)`. ########## tests/scripts/benchmark_kwargs_wrapper.py: ########## @@ -0,0 +1,93 @@ +# 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. +"""Benchmark API overhead of kwargs wrapper. + +This benchmark compares the overhead of using make_kwargs_wrapper to add +keyword argument support to a positional-only function. We benchmark a +function that accepts three torch tensors where the last two have defaults. Review Comment:  The docstring here and other comments in the file state that the benchmark uses `torch` tensors, but the implementation uses simple integers. This discrepancy can be misleading for future readers of the code. Since using integers is sufficient for measuring call overhead, the documentation should be updated to reflect this. ```suggestion This benchmark compares the overhead of using make_kwargs_wrapper to add keyword argument support to a positional-only function. We benchmark a function that accepts three integer arguments where the last two have 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]
