gemini-code-assist[bot] commented on code in PR #395:
URL: https://github.com/apache/tvm-ffi/pull/395#discussion_r2678008726
##########
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 could be
more comprehensive. It would be beneficial to also test the other branches of
the `__init__` implementation:
1. Passing an existing `tvm_ffi.Function` instance.
2. Passing a non-callable argument, which should raise a `TypeError`.
Adding these cases would ensure the new constructor is fully tested.
```suggestion
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
# Test initializing from an existing tvm_ffi.Function
fadd2 = tvm_ffi.Function(fadd)
assert isinstance(fadd2, tvm_ffi.Function)
assert fadd2(3, 4) == 7
assert fadd is not fadd2
assert fadd.same_as(fadd2)
# Test with non-callable
with pytest.raises(TypeError, match="func must be callable"):
tvm_ffi.Function(123)
```
##########
python/tvm_ffi/cython/function.pxi:
##########
@@ -889,6 +889,26 @@ 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.
+
+ Parameters
+ ----------
+ func : Callable[..., Any]
+ Optional Python callable to wrap. If provided, creates an FFI
+ function that wraps this callable. If None, the Function is
+ expected to be initialized via static factory methods.
+ """
Review Comment:
The docstring for `__init__` is a bit misleading. The `func` parameter is
not optional, and passing `None` will raise a `TypeError` because
`callable(None)` is `False`. The mention of "Optional" and "If None..." could
confuse users. I suggest clarifying that `func` is a required argument.
```
"""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.
"""
```
--
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]
