gemini-code-assist[bot] commented on code in PR #49:
URL: https://github.com/apache/tvm-ffi/pull/49#discussion_r2377254863
##########
python/tvm_ffi/cython/object.pxi:
##########
@@ -261,29 +263,54 @@ cdef inline object make_ret_opaque_object(TVMFFIAny
result):
(<Object>obj).chandle = result.v_obj
return obj.pyobject()
+cdef inline object make_fallback_cls_for_type_index(int32_t type_index):
+ cdef str type_key = _type_index_to_key(type_index)
+ cdef object type_info = _lookup_type_info_from_type_key(type_key)
+ cdef object parent_type_info = type_info.parent_type_info
+
+ class cls(parent_type_info.type_cls):
+ pass
Review Comment:
If an object of a derived type is returned, but neither the derived type nor
its base type are registered on the Python side, `parent_type_info.type_cls`
will be `None`. This will cause a `TypeError` when attempting to use it as a
base class at line 271. A recursive call to `make_fallback_cls_for_type_index`
for the parent type is needed to ensure the parent class is created first.
```
cdef object parent_type_info = type_info.parent_type_info
if parent_type_info.type_cls is None:
make_fallback_cls_for_type_index(parent_type_info.type_index)
class cls(parent_type_info.type_cls):
pass
```
##########
python/tvm_ffi/cython/type_info.pxi:
##########
@@ -101,6 +102,21 @@ class TypeMethod:
func: object
is_static: bool
+ def as_callable(self, object cls):
+ """Create a Python method attribute for this method on ``cls``."""
+ cdef str name = self.name
+ cdef str doc = self.doc or f"Method `{name}` of class
`{cls.__qualname__}`"
+ cdef object func = self.func
+ if self.is_static:
+ func = staticmethod(func)
+ else:
+ func = _member_method_wrapper(func)
+ func.__module__ = cls.__module__
+ func.__name__ = name
+ func.__qualname__ = f"{cls.__qualname__}.{name}"
+ func.__doc__ = doc
+ return func
Review Comment:
Setting attributes like `__doc__` on a `staticmethod` object will raise an
`AttributeError` at runtime. These attributes should be set on the function
object *before* it's wrapped by `staticmethod`. For non-static methods, the
attributes should be set on the wrapper function. The suggested change
refactors the logic to correctly handle both cases.
```
cdef object func
if self.is_static:
func = self.func
else:
func = _member_method_wrapper(self.func)
func.__module__ = cls.__module__
func.__name__ = name
func.__qualname__ = f"{cls.__qualname__}.{name}"
func.__doc__ = doc
if self.is_static:
return staticmethod(func)
return func
```
##########
python/tvm_ffi/cython/object.pxi:
##########
@@ -261,29 +263,54 @@ cdef inline object make_ret_opaque_object(TVMFFIAny
result):
(<Object>obj).chandle = result.v_obj
return obj.pyobject()
+cdef inline object make_fallback_cls_for_type_index(int32_t type_index):
+ cdef str type_key = _type_index_to_key(type_index)
+ cdef object type_info = _lookup_type_info_from_type_key(type_key)
+ cdef object parent_type_info = type_info.parent_type_info
+
+ class cls(parent_type_info.type_cls):
+ pass
+
+ assert type_info.type_cls is None
+ assert parent_type_info is not None
+
+ attrs = dict(cls.__dict__)
+ attrs.pop("__dict__", None)
+ attrs.pop("__weakref__", None)
+ attrs["__slots__"] = ()
+ attrs["__tvm_ffi_type_info__"] = type_info
+ for field in type_info.fields:
+ attrs[field.name] = field.as_property(cls)
+ for method in type_info.methods:
+ name = method.name
+ if name == "__ffi_init__":
+ name = "__c_ffi_init__"
+ attrs[name] = method.as_callable(cls)
+ for name, val in attrs.items():
+ setattr(cls, name, val)
+ type_info.type_cls = cls
+ _update_registry(type_index, type_key, type_info, cls)
+ return cls
+
cdef inline object make_ret_object(TVMFFIAny result):
- global TYPE_INDEX_TO_INFO
- cdef int32_t tindex
- cdef object cls
- tindex = result.type_index
-
- if tindex < len(TYPE_INDEX_TO_CLS):
- cls = TYPE_INDEX_TO_CLS[tindex]
- if cls is not None:
- if issubclass(cls, PyNativeObject):
- obj = Object.__new__(Object)
- (<Object>obj).chandle = result.v_obj
- return cls.__from_tvm_ffi_object__(cls, obj)
- obj = cls.__new__(cls)
- (<Object>obj).chandle = result.v_obj
- return obj
+ cdef int32_t type_index
+ cdef object cls, obj
+ type_index = result.type_index
- # object is not found in registered entry
- # in this case we need to report an warning
- type_key = _type_index_to_key(tindex)
- warnings.warn(f"Returning type `{type_key}` which is not registered via
register_object, fallback to Object")
- obj = _CLASS_OBJECT.__new__(_CLASS_OBJECT)
+ if type_index < len(TYPE_INDEX_TO_CLS) and (cls :=
TYPE_INDEX_TO_CLS[type_index]) is not None:
+ if issubclass(cls, PyNativeObject):
+ obj = Object.__new__(Object)
+ (<Object>obj).chandle = result.v_obj
+ return cls.__from_tvm_ffi_object__(cls, obj)
+ else:
+ pass
Review Comment:
The `else: pass` block is redundant and can be removed to improve code
clarity. The logic will correctly fall through to the object creation for
non-`PyNativeObject` types.
--
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]
