Skip to content

Conversation

@junrushao
Copy link
Member

Per discussion w/ @tqchen.

In dataclass-syntax classes as introduced in #8, we likely always use the reflected _C___init__ method to initialize an instance. This PR gives a shortcut to this convention, where

self.__init_handle_by_constructor__(_MyClass._C___init__, ...)

is equivalent to

self.__ffi_init__(v_i64 + 1, v_i32 + 2)

@junrushao junrushao marked this pull request as ready for review September 19, 2025 20:59
@junrushao junrushao requested review from Copilot and tqchen and removed request for tqchen September 19, 2025 20:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a convenience initializer method to mirror calling the reflected C++ _C___init__ constructor and exposes an additional field on TypeField.

  • Introduces Object.ffi_init as a shortcut to init_handle_by_constructor(type(self).C___init_, ...).
  • Updates type stubs accordingly and adds a new dataclass_field attribute to TypeField.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
python/tvm_ffi/cython/object.pxi Adds ffi_init convenience wrapper with docstring.
python/tvm_ffi/core.pyi Adds stub for ffi_init and new TypeField.dataclass_field attribute.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +49 to +54
"""Initialize the instance using the ` __init__` method registered on C++ side.
Parameters
----------
args: list of objects
The arguments to the constructor
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Same docstring issues as implementation: backticked name contains an extra space ( __init__) and parameter description should reflect variadic positional arguments rather than 'list of objects'. Mirror the corrected wording in the implementation for consistency.

Suggested change
"""Initialize the instance using the ` __init__` method registered on C++ side.
Parameters
----------
args: list of objects
The arguments to the constructor
"""Initialize the instance using the `__init__` method registered on the C++ side.
Parameters
----------
args
Positional arguments to pass to the constructor.

Copilot uses AI. Check for mistakes.
offset: int
frozen: bool
getter: Any
setter: Any
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] New attribute dataclass_field is added without any doc/comment explaining its purpose or when it may be None. Consider adding a brief class-level note or inline comment to clarify its role (e.g., link to dataclass reflection mechanism) to aid future maintainability.

Suggested change
setter: Any
setter: Any
# Reference to the corresponding dataclass field, if the FFI-backed type is also a Python dataclass.
# May be None if no dataclass field is associated (e.g., for non-dataclass types).

Copilot uses AI. Check for mistakes.
@tqchen
Copy link
Member

tqchen commented Sep 19, 2025

minor nit, on c function naming, how about __c_init__ for the pattern, it aligns with other dlpack style dunder naming(see dmlc/dlpack#175, also shorter to type in def_static

@junrushao
Copy link
Member Author

Merged this PR into #8

@junrushao junrushao closed this Sep 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants