gemini-code-assist[bot] commented on code in PR #448:
URL: https://github.com/apache/tvm-ffi/pull/448#discussion_r2808112498
##########
tests/python/test_function.py:
##########
@@ -153,6 +153,15 @@ def fapply(f: Any, *args: Any) -> Any:
assert fapply(add, 1, 3.3) == 4.3
+def test_pyfunc_init() -> None:
+ def add(a: int, b: int) -> int:
+ return a + b
+
+ fadd = tvm_ffi.Function(add)
+ assert isinstance(fadd, tvm_ffi.Function)
+ assert fadd(1, 2) == 3
Review Comment:
The test for `tvm_ffi.Function.__init__` is a good start, but it only covers
creating a `Function` from a new Python callable. To ensure the new constructor
is fully tested, please consider adding test cases for:
1. Creating a `Function` from an existing `tvm_ffi.Function` instance.
2. Verifying that creating a `Function` from a non-callable object
correctly raises a `TypeError`.
```suggestion
# Test creating from a Python callable
fadd = tvm_ffi.Function(add)
assert isinstance(fadd, tvm_ffi.Function)
assert fadd(1, 2) == 3
# Test creating from an existing tvm_ffi.Function
fadd2 = tvm_ffi.Function(fadd)
assert isinstance(fadd2, tvm_ffi.Function)
assert fadd2(3, 4) == 7
assert fadd.same_as(fadd2)
# Test creating from a non-callable raises TypeError
with pytest.raises(TypeError):
tvm_ffi.Function(123)
```
##########
python/tvm_ffi/cython/function.pxi:
##########
@@ -889,6 +889,27 @@ cdef class Function(Object):
def __cinit__(self) -> None:
self.c_release_gil = _RELEASE_GIL_BY_DEFAULT
+ def __init__(self, func: Callable[..., Any]) -> None:
+ """Initialize a Function from a Python callable.
+
+ This constructor allows creating a `tvm_ffi.Function` directly
+ from a Python function or another `tvm_ffi.Function` instance.
+
+ Parameters
+ ----------
+ func : Callable[..., Any]
+ The Python callable to wrap.
+ """
+ cdef TVMFFIObjectHandle chandle = NULL
+ if not callable(func):
+ raise TypeError(f"func must be callable, got {type(func)}")
+ if isinstance(func, Function):
+ chandle = (<Object>func).chandle
+ TVMFFIObjectIncRef(chandle)
+ else:
+ _convert_to_ffi_func_handle(func, &chandle)
Review Comment:
The logic for handling different types of `func` can be restructured for
better clarity and a minor efficiency gain. Since `isinstance(func, Function)`
is a more specific check than `callable(func)`, it's better to check for it
first. Using a single `if/elif/else` block makes the different cases clearer.
```
if isinstance(func, Function):
chandle = (<Object>func).chandle
TVMFFIObjectIncRef(chandle)
elif callable(func):
_convert_to_ffi_func_handle(func, &chandle)
else:
raise TypeError(f"func must be callable, got {type(func)}")
```
--
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]
