[Stacked PR] Introduce @tvm_ffi.dataclasses.py_class#499
Draft
junrushao wants to merge 12 commits intoapache:mainfrom
Draft
[Stacked PR] Introduce @tvm_ffi.dataclasses.py_class#499junrushao wants to merge 12 commits intoapache:mainfrom
@tvm_ffi.dataclasses.py_class#499junrushao wants to merge 12 commits intoapache:mainfrom
Conversation
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the TVM FFI system by introducing the ability to define FFI objects directly within Python using a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces @tvm_ffi.dataclasses.py_class, a powerful decorator for defining FFI-compatible classes in Python with dataclass-like semantics. This is a substantial feature that includes a sophisticated two-phase registration mechanism to handle forward references, a comprehensive type checking and conversion system built around TypeSchema, and automatic generation of dunder methods like __init__ and __repr__. The changes span C++, Cython, and Python, and appear to be well-engineered and robust. My review found one area for improvement in error reporting for unresolved type hints. Overall, this is an excellent and significant contribution to TVM's FFI capabilities.
Note: Security Review did not run due to the size of the PR.
Comment on lines
+276
to
+290
| def _raise_unresolved_forward_reference(cls: type, globalns: dict[str, Any]) -> None: | ||
| """Raise :class:`TypeError` listing the annotations that cannot be resolved.""" | ||
| localns = dict(_PY_CLASS_BY_MODULE.get(cls.__module__, {})) | ||
| localns[cls.__name__] = cls | ||
| unresolved: list[str] = [] | ||
| for name, ann_str in getattr(cls, "__annotations__", {}).items(): | ||
| if isinstance(ann_str, str): | ||
| try: | ||
| eval(ann_str, globalns, localns) | ||
| except NameError: | ||
| unresolved.append(f"{name}: {ann_str}") | ||
| raise TypeError( | ||
| f"Cannot instantiate {cls.__name__}: unresolved forward references: {unresolved}" | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
The _raise_unresolved_forward_reference function is designed to provide a helpful error message when type hint resolution fails. However, it only checks for NameError when trying to eval string annotations. The typing.get_type_hints function can fail for other reasons, such as AttributeError (e.g., for an annotation like 'foo.Bar' where foo exists but Bar does not). In such cases, this function might not identify any unresolved references and produce a confusing error message with an empty list of unresolved references.
To make the error reporting more robust, you could consider catching a broader range of exceptions (like Exception) during the eval attempt and reporting the annotation string that caused any failure, not just a NameError.
…patch Add kTVMFFIFieldFlagBitSetterIsFunctionObj (1 << 11) flag to TVMFFIFieldFlags. When set, the setter member of TVMFFIFieldInfo is a TVMFFIObjectHandle pointing to a FunctionObj instead of a raw TVMFFIFieldSetter function pointer. Change the setter field type from TVMFFIFieldSetter to void* to accommodate both variants. Introduce a CallFieldSetter helper in accessor.h that centralizes the dispatch logic. Update all C++ call sites (init.h, creator.h, serialization.cc, reflection_extra.cc), Cython bindings (base.pxi, type_info.pxi, object.pxi, tvm_ffi_python_helpers.h), and the Rust FFI struct (c_api.rs) to use the new dispatch path. BREAKING CHANGE: TVMFFIFieldInfo::setter is now void* instead of TVMFFIFieldSetter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…aders Fix namespace resolution ambiguity when reflection headers are included from code within namespace tvm::ffi::reflection (which has its own details sub-namespace). Use fully-qualified ::tvm::ffi::details:: paths for ObjectUnsafe, AnyUnsafe, FunctionInfo, TypeSchemaImpl, TypeSchema, OverloadedFunction, CreateNewOverload, and OverloadBase references. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add CreateEmptyObject() and HasCreator() inline helpers to function.h that unify the scattered creator-or-error patterns across the reflection subsystem. The new fast path calls the native C++ creator; when that is NULL (Python-defined types), it falls back to the __ffi_new__ type attribute. Simplify five call-sites (ObjectCreator, MakeInit, MakeObjectFromPackedArgs, ObjectGraphDeserializer) to delegate to CreateEmptyObject instead of duplicating the creator-null check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rsion Add CastFromAny<TObjectRef> template in reflection::details that performs AnyView -> ObjectRef conversion through TypeTraits. Add RegisterConvertTypeAttr<T>() and ObjectDef::ref<TObjectRef>() to register __ffi_convert__ type attributes for reflected types. Register __ffi_convert__ for all core types: Object, String, Bytes, Error, Function, Shape, Tensor, Array, Map, List, Dict. Initialize structural_eq_hash_kind in base Object metadata. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add C++ infrastructure for Python-defined FFI types: - PyClassDeleter: destructor for calloc-allocated Python-defined objects - GetFieldGetter/WriteFieldValue: generic field access by type index - MakeFieldSetter: creates FunctionObj setter with Cython callback - MakeFFINew: allocates + registers __ffi_new__ and __ffi_shallow_copy__ Register new global functions: ffi.GetFieldGetter, ffi.MakeFieldSetter, ffi.MakeFFINew, ffi.RegisterAutoInit, ffi.FunctionFromExternC. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add Cython declarations for: - TVMFFITypeGetOrAllocIndex, TVMFFITypeRegisterField/Metadata/Attr, TVMFFIErrorSetRaisedFromCStr (type registration from Python) - Additional field flags: SEqHashIgnore, SEqHashDef, ReprOff, CompareOff, HashOff - TVMFFISEqHashKind enum - Fix TVMFFITypeMetadata.total_size type (int64_t -> int32_t) and add structural_eq_hash_kind field to match C struct layout - Add cause_chain and extra_context fields to error info struct Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add CAny cdef class (owned TVMFFIAny container) with to_py() and proper ref-counting in object.pxi. Enhance TypeSchema in type_info.pxi: - Add origin_type_index field for resolved type index lookup - Add from_annotation() static method for Python type annotations - Add from_type_index() static method - Add to_json() for JSON-round-trippable type descriptors - Add _ORIGIN_TO_TYPE_INDEX and _TYPE_INDEX_TO_ORIGIN dicts - Add STL type converters to _TYPE_SCHEMA_ORIGIN_CONVERTER - Add TypeField.ty optional TypeSchema attribute - Add TypeInfo.total_size cached_property - Make TypeInfo.fields Optional[list[TypeField]] Fix _lookup_type_attr to copy AnyView to owned Any. Export CAny from tvm_ffi.__init__.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add type_converter.pxi implementing _TypeConverter cdef class with C function pointer dispatch for TypeSchema type conversion. Each converter compiles to a single indirect C function call with zero Python attribute lookup. Wire into TypeSchema via check_value() and convert() methods that return CAny. Add _converter cached_property for lazy builder. Key features: - 20+ leaf/composite converter functions for POD, Object, Optional, Union, Array, List, Map, Dict, tuple, Callable, Any - __ffi_convert__ type attribute delegation for object types - Iterative __tvm_ffi_value__ protocol with depth limit (64) - Zero-copy container fast path (copy-on-first-change) - Exception-based error signaling via _ConvertError Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add test_type_converter.py with 80+ test classes covering: - POD type exact match and implicit conversions - Object type hierarchy and custom objects - Optional, Union, and nested container types - Marshal protocols (__tvm_ffi_value__, __tvm_ffi_object__, etc.) - Cycle detection and depth limits - __ffi_convert__ registration and dispatch - CAny wrapper operations - TypeSchema.from_annotation for all Python type annotations - Zero-copy conversion paths - Error message quality and exception normalization Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…iptor Add Python-defined type registration infrastructure: Cython (type_info.pxi): - TypeInfo._register_fields() and _register_methods() instance methods - _ORIGIN_NATIVE_LAYOUT dict for field size/alignment computation - _register_one_field cdef for building and registering TVMFFIFieldInfo - _f_type_convert C callback for type conversion in setters - _register_fields() module-level function orchestrating full registration Cython (object.pxi): - _register_py_class(): allocate dynamic type index, register in tables - _rollback_py_class(): undo registration on validation failure Python (field.py): - Field descriptor class matching dataclasses.field() API - field() helper function, KW_ONLY sentinel Python (registry.py): - Wire __post_init__ into FFI object init path - Add dunder family-skip logic (eq/ne and ordering families) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tvm_ffi.dataclasses.py_class, a Python dataclass-style decorator that parses class annotations into FFI fields without requiring a C++ type definition. Key capabilities: - @py_class decorator parsing class annotations into FFI fields - field() helper matching the dataclasses.field() API - Deferred forward-reference resolution for mutual/self-referential types - __post_init__ support - Automatic init-signature reordering (required before optional) - Copy-based __replace__ - KW_ONLY sentinel for keyword-only fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add test_dataclass_py_class.py with 50+ test classes covering: - Basic registration and field parsing - Defaults, kw_only, ClassVar handling - Init signature, __post_init__, repr, equality, ordering, hash - Deep copy and inheritance - Forward references and mutual references - Field API and edge cases - Hash tri-state semantics - Native parent inheritance (C++ -> Python) - Type conversion errors and setter/getter corner cases - Bool alignment and memory lifetime - FFI global function existence checks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
junrushao
force-pushed
the
2026-03-09/py-class-support
branch
from
7d91fb0 to
46a21b7
Compare
tqchen
reviewed
Mar 10, 2026
| * \return An owned ObjectPtr to the newly allocated (zero-initialized) object. | ||
| * \throws RuntimeError if neither creator nor __ffi_new__ is available. | ||
| */ | ||
| inline ObjectPtr<Object> CreateEmptyObject(const TVMFFITypeInfo* type_info) { |
Member
There was a problem hiding this comment.
this should go into reflection/Creator
tqchen
reviewed
Mar 10, 2026
| // Release FunctionObj setter handles that were IncRef'd during RegisterTypeField. | ||
| for (TVMFFIFieldInfo& field : type_fields_data) { | ||
| if ((field.flags & kTVMFFIFieldFlagBitSetterIsFunctionObj) && field.setter != nullptr) { | ||
| TVMFFIObjectDecRef(static_cast<TVMFFIObjectHandle>(field.setter)); |
Member
There was a problem hiding this comment.
if we do not-realloc entry, consider use any_pool, the semantics should be when user pass the function in, the ObjectRef is not owned, and we need to push to any_pool_ to increase the ref count
tqchen
reviewed
Mar 10, 2026
| * \tparam T The C++ type stored at the field address. | ||
| */ | ||
| template <typename T> | ||
| int PyClassFieldGetter(void* field, TVMFFIAny* result) { |
Member
There was a problem hiding this comment.
pyclass helpers can go into cython/pyclass_helpers.h?
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.