gemini-code-assist[bot] commented on code in PR #36:
URL: https://github.com/apache/tvm-ffi/pull/36#discussion_r2392761387
##########
python/tvm_ffi/cython/type_info.pxi:
##########
@@ -58,6 +59,80 @@ cdef class FieldSetter:
raise_existing_error()
raise move_from_last_error().py_error()
+_TYPE_SCHEMA_ORIGIN_CONVERTER = {
+ # A few Python-native types
+ "Variant": "Union",
+ "Optional": "Optional",
+ "Tuple": "tuple",
+ "ffi.Function": "Callable",
+ "ffi.Array": "list",
+ "ffi.Map": "dict",
+ "ffi.OpaquePyObject": "Any",
+ # ctype types
+ "void*": "ctypes.c_void_p",
+ # bytes
+ "TVMFFIByteArray*": "bytes",
+ "ffi.SmallBytes": "bytes",
+ "ffi.Bytes": "bytes",
+ # strings
+ "const char*": "str",
+ "ffi.SmallStr": "str",
+ "ffi.String": "str",
Review Comment:
The `_TYPE_SCHEMA_ORIGIN_CONVERTER` dictionary is missing a mapping for
`"std::string"` to `str`. C++ functions that take `std::string` as an argument
will have `{"type":"std::string"}` as their schema. Without this mapping, the
Python `TypeSchema` will have an origin of `"std::string"`, which is not a
standard Python type. To provide more idiomatic Python type hints,
`"std::string"` should also be converted to `str`.
```
"const char*": "str",
"std::string": "str",
"ffi.SmallStr": "str",
```
##########
python/tvm_ffi/cython/type_info.pxi:
##########
@@ -58,6 +59,80 @@ cdef class FieldSetter:
raise_existing_error()
raise move_from_last_error().py_error()
+_TYPE_SCHEMA_ORIGIN_CONVERTER = {
+ # A few Python-native types
+ "Variant": "Union",
+ "Optional": "Optional",
+ "Tuple": "tuple",
+ "ffi.Function": "Callable",
+ "ffi.Array": "list",
+ "ffi.Map": "dict",
+ "ffi.OpaquePyObject": "Any",
+ # ctype types
+ "void*": "ctypes.c_void_p",
+ # bytes
+ "TVMFFIByteArray*": "bytes",
+ "ffi.SmallBytes": "bytes",
+ "ffi.Bytes": "bytes",
+ # strings
+ "const char*": "str",
+ "ffi.SmallStr": "str",
+ "ffi.String": "str",
+}
+
+
[email protected](repr=False, frozen=True)
+class TypeSchema:
+ """Type schema for a TVM FFI type."""
+ origin: str
+ args: tuple[TypeSchema, ...] = ()
+
+ def __post_init__(self):
+ origin = self.origin
+ args = self.args
+ if origin == "Union":
+ assert len(args) >= 2, "Union must have at least two arguments"
+ elif origin == "Optional":
+ assert len(args) == 1, "Optional must have exactly one argument"
+ elif origin == "Callable":
+ assert len(args) == 0 or len(args) >= 1, "Callable must have at
least one argument"
Review Comment:
The assertion `len(args) == 0 or len(args) >= 1` is always true for any
tuple `args`, as its length is always non-negative. This makes the assertion
redundant and potentially confusing. The logic in `__repr__` already handles
the cases for typed and untyped callables correctly. I suggest removing this
assertion logic to improve clarity.
```
elif origin == "Callable":
pass
```
##########
include/tvm/ffi/container/array.h:
##########
@@ -1135,6 +1135,13 @@ struct TypeTraits<Array<T>> : public
ObjectRefTypeTraitsBase<Array<T>> {
}
TVM_FFI_INLINE static std::string TypeStr() { return "Array<" +
details::Type2Str<T>::v() + ">"; }
+ TVM_FFI_INLINE static std::string TypeSchemaJson() {
+ std::ostringstream oss;
+ oss << "{\"type\":\"" << StaticTypeKey::kTVMFFIArray << "\",\"args\":[";
Review Comment:
The `TypeSchemaJson` for `Array<T>` uses `StaticTypeKey::kTVMFFIArray`,
which resolves to `"Array"`. However, the registered type key for `Array` is
`"ffi.Array"`. This mismatch will cause issues on the Python side when parsing
the schema, as the converter expects `"ffi.Array"`. The schema should use the
registered type key for consistency. A similar issue exists for `Map`,
`Function`, `String`, and `Bytes`.
```c
oss << "{\"type\":\"ffi.Array\",\"args\":[";
```
--
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]
