gemini-code-assist[bot] commented on code in PR #311:
URL: https://github.com/apache/tvm-ffi/pull/311#discussion_r2590188646
##########
python/tvm_ffi/utils/kwargs_wrapper.py:
##########
@@ -276,17 +279,18 @@ def wrapper({arg_list}):
exec(code_str, exec_globals, namespace)
new_func = namespace["wrapper"]
- # Copy metadata from prototype_func if provided
- if prototype_func is not None:
- functools.update_wrapper(new_func, prototype_func, updated=())
+ # Copy metadata from prototype if provided
+ if prototype is not None:
+ functools.update_wrapper(new_func, prototype, updated=())
return new_func
def make_kwargs_wrapper_from_signature(
target_func: Callable,
signature: inspect.Signature,
- prototype_func: Callable | None = None,
+ prototype: Callable | None = None,
+ exclude_arg_names: list[str] | None = None,
Review Comment:
For better API flexibility, consider using `typing.Iterable[str]` for the
`exclude_arg_names` type hint instead of `list[str]`. The implementation
`set(exclude_arg_names)` already supports any iterable, and this change would
make the type hint accurately reflect that, allowing users to pass tuples or
other iterables without causing type checker warnings. This would require
importing `Iterable` from `typing`.
##########
tests/python/utils/test_kwargs_wrapper.py:
##########
@@ -218,6 +222,36 @@ def with_kwargs(a: Any, **kwargs: Any) -> None:
with pytest.raises(ValueError, match=r"\*\*kwargs not supported"):
make_kwargs_wrapper_from_signature(target,
inspect.signature(with_kwargs))
+ # Test exclude_arg_names - ignore certain arguments from the signature
+ def source_with_skip(a: Any, b: Any, c: int = 10, d: int = 20) -> None:
+ pass
+
+ wrapper = make_kwargs_wrapper_from_signature(
+ target, inspect.signature(source_with_skip), exclude_arg_names=["c"]
+ )
+ # c is ignored, so wrapper should only have a, b, d
+ assert wrapper(1, 2) == 23 # 1 + 2 + 20 (d=20)
+ assert wrapper(1, 2, d=5) == 8 # 1 + 2 + 5
+
+ # Test ignoring multiple arguments
+ wrapper = make_kwargs_wrapper_from_signature(
+ target, inspect.signature(source_with_skip), exclude_arg_names=["b",
"d"]
+ )
+ # b and d are ignored, so wrapper should only have a, c
+ assert wrapper(1) == 11 # 1 + 10 (c=10)
+ assert wrapper(1, c=5) == 6 # 1 + 5
+
+ # Test ignoring keyword-only arguments
+ def source_kwonly_skip(a: Any, b: Any, *, c: int = 10, d: int = 20) ->
None:
+ pass
+
+ wrapper = make_kwargs_wrapper_from_signature(
+ target, inspect.signature(source_kwonly_skip), exclude_arg_names=["c"]
+ )
+ # c is skipped, so wrapper should only have a, b, d
+ assert wrapper(1, 2) == 23 # 1 + 2 + 20 (d=20)
+ assert wrapper(1, 2, d=5) == 8 # 1 + 2 + 5
Review Comment:
It would be beneficial to add a test case to verify the behavior when
`exclude_arg_names` contains names that are not present in the source
function's signature. This would document that non-existent argument names are
silently ignored, which is the current reasonable behavior.
For example:
```python
# Test excluding a non-existent argument
wrapper = make_kwargs_wrapper_from_signature(
target, inspect.signature(source_with_skip),
exclude_arg_names=["non_existent"]
)
# Should be the same as no exclusion
assert wrapper(1, 2) == 33
```
--
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]
