Re: [PR] feat(python): auto-register __ffi_* dunder methods in @py_class as TypeMethod [tvm-ffi]
junrushao merged PR #508: URL: https://github.com/apache/tvm-ffi/pull/508 -- 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]
Re: [PR] feat(python): auto-register __ffi_* dunder methods in @py_class as TypeMethod [tvm-ffi]
junrushao commented on PR #508: URL: https://github.com/apache/tvm-ffi/pull/508#issuecomment-4106929549 /gemini review -- 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]
Re: [PR] feat(python): auto-register __ffi_* dunder methods in @py_class as TypeMethod [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #508: URL: https://github.com/apache/tvm-ffi/pull/508#discussion_r2972045484 ## python/tvm_ffi/dataclasses/py_class.py: ## @@ -202,6 +202,30 @@ def _collect_own_fields( return fields +def _collect_py_methods(cls: type) -> list[tuple[str, Any, bool]] | None: +"""Extract recognized FFI dunder methods from the class body. + +Only names listed in :data:`_FFI_RECOGNIZED_METHODS` are collected. + +Returns a list of ``(name, func, is_static)`` tuples, or ``None`` +if no eligible methods were found. +""" +methods: list[tuple[str, Any, bool]] = [] +for name, value in cls.__dict__.items(): +if name not in _FFI_RECOGNIZED_METHODS: +continue +if isinstance(value, staticmethod): +func = value.__func__ +is_static = True +elif callable(value): +func = value +is_static = False +else: +continue +methods.append((name, func, is_static)) +return methods if methods else None Review Comment:  This is a great addition! To further improve introspection, consider extracting the docstring from the collected methods and passing it to the FFI layer. This would allow the docstrings of Python-defined FFI methods to be visible in other languages. You can use `inspect.getdoc()` for this (you'll need to `import inspect`). This would require corresponding changes in `python/tvm_ffi/cython/type_info.pxi`: 1. Update `TypeInfo._register_py_methods` to expect a `(name, func, is_static, doc)` tuple. 2. Update the cdef function `_register_py_methods` to unpack the docstring and populate `method_info.doc` before calling `TVMFFITypeRegisterMethod`. ```python def _collect_py_methods(cls: type) -> list[tuple[str, Any, bool, str | None]] | None: """Extract recognized FFI dunder methods from the class body. Only names listed in :data:`_FFI_RECOGNIZED_METHODS` are collected. Returns a list of ``(name, func, is_static, doc)`` tuples, or ``None`` if no eligible methods were found. """ methods: list[tuple[str, Any, bool, str | None]] = [] for name, value in cls.__dict__.items(): if name not in _FFI_RECOGNIZED_METHODS: continue if isinstance(value, staticmethod): func = value.__func__ is_static = True elif callable(value): func = value is_static = False else: continue doc = inspect.getdoc(func) methods.append((name, func, is_static, doc)) return methods if methods else None ``` -- 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]
Re: [PR] feat(python): auto-register __ffi_* dunder methods in @py_class as TypeMethod [tvm-ffi]
junrushao commented on code in PR #508: URL: https://github.com/apache/tvm-ffi/pull/508#discussion_r2971877489 ## python/tvm_ffi/cython/type_info.pxi: ## @@ -996,6 +1014,64 @@ def _register_fields(type_info, fields): return type_fields +cdef _register_py_methods(int32_t type_index, list py_methods): +"""Register user-defined dunder methods as both TypeMethod and TypeAttr. + +For each method in *py_methods*: +1. Convert the Python callable to a ``TVMFFIAny`` (``ffi::Function``). +2. Call ``TVMFFITypeRegisterMethod`` so the method appears in the + type's reflection metadata (``TypeInfo.methods``). +3. Ensure the type-attribute column exists (sentinel call with + ``type_index = kTVMFFINone``), then call ``TVMFFITypeRegisterAttr`` + so the C++ runtime dispatch can find the hook. + +Parameters +-- +type_index : int +The runtime type index of the type. +py_methods : list[tuple[str, callable, bool]] +Each entry is ``(name, func, is_static)``. +""" +cdef TVMFFIMethodInfo method_info +cdef TVMFFIAny func_any +cdef int c_api_ret_code +cdef ByteArrayArg name_arg +cdef TVMFFIAny sentinel_any + +sentinel_any.type_index = kTVMFFINone +sentinel_any.v_int64 = 0 + +for name, func, is_static in py_methods: +name_bytes = c_str(name) +name_arg = ByteArrayArg(name_bytes) + +# Convert Python callable -> TVMFFIAny (creates a FunctionObj) +func_any.type_index = kTVMFFINone +func_any.v_int64 = 0 +TVMFFIPyPyObjectToFFIAny( +TVMFFIPyArgSetterFactory_, +func, +&func_any, +&c_api_ret_code, +) +CHECK_CALL(c_api_ret_code) + +# 1. Register as TypeMethod +method_info.name = name_arg.cdata +method_info.doc.data = NULL +method_info.doc.size = 0 +method_info.flags = kTVMFFIFieldFlagBitMaskIsStaticMethod if is_static else 0 +method_info.method = func_any +method_info.metadata.data = NULL +method_info.metadata.size = 0 +CHECK_CALL(TVMFFITypeRegisterMethod(type_index, &method_info)) + +# 2. Ensure type-attr column exists (sentinel: kTVMFFINone) +CHECK_CALL(TVMFFITypeRegisterAttr(kTVMFFINone, &name_arg.cdata, &sentinel_any)) +# 3. Register as TypeAttr +CHECK_CALL(TVMFFITypeRegisterAttr(type_index, &name_arg.cdata, &func_any)) Review Comment: good catch! ## python/tvm_ffi/dataclasses/py_class.py: ## @@ -202,6 +202,31 @@ def _collect_own_fields( return fields +def _collect_py_methods(cls: type) -> list[tuple[str, Any, bool]] | None: +"""Extract recognized FFI dunder methods from the class body. + +Only names listed in :data:`_FFI_RECOGNIZED_METHODS` are collected. + +Returns a list of ``(name, func, is_static)`` tuples, or ``None`` +if no eligible methods were found. +""" +methods: list[tuple[str, Any, bool]] = [] +for name in list(cls.__dict__): +if name not in _FFI_RECOGNIZED_METHODS: +continue +value = cls.__dict__[name] Review Comment: good catch! -- 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]
Re: [PR] feat(python): auto-register __ffi_* dunder methods in @py_class as TypeMethod [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #508: URL: https://github.com/apache/tvm-ffi/pull/508#discussion_r2971866468 ## python/tvm_ffi/cython/type_info.pxi: ## @@ -996,6 +1014,64 @@ def _register_fields(type_info, fields): return type_fields +cdef _register_py_methods(int32_t type_index, list py_methods): +"""Register user-defined dunder methods as both TypeMethod and TypeAttr. + +For each method in *py_methods*: +1. Convert the Python callable to a ``TVMFFIAny`` (``ffi::Function``). +2. Call ``TVMFFITypeRegisterMethod`` so the method appears in the + type's reflection metadata (``TypeInfo.methods``). +3. Ensure the type-attribute column exists (sentinel call with + ``type_index = kTVMFFINone``), then call ``TVMFFITypeRegisterAttr`` + so the C++ runtime dispatch can find the hook. + +Parameters +-- +type_index : int +The runtime type index of the type. +py_methods : list[tuple[str, callable, bool]] +Each entry is ``(name, func, is_static)``. +""" +cdef TVMFFIMethodInfo method_info +cdef TVMFFIAny func_any +cdef int c_api_ret_code +cdef ByteArrayArg name_arg +cdef TVMFFIAny sentinel_any + +sentinel_any.type_index = kTVMFFINone +sentinel_any.v_int64 = 0 + +for name, func, is_static in py_methods: +name_bytes = c_str(name) +name_arg = ByteArrayArg(name_bytes) + +# Convert Python callable -> TVMFFIAny (creates a FunctionObj) +func_any.type_index = kTVMFFINone +func_any.v_int64 = 0 +TVMFFIPyPyObjectToFFIAny( +TVMFFIPyArgSetterFactory_, +func, +&func_any, +&c_api_ret_code, +) +CHECK_CALL(c_api_ret_code) + +# 1. Register as TypeMethod +method_info.name = name_arg.cdata +method_info.doc.data = NULL +method_info.doc.size = 0 +method_info.flags = kTVMFFIFieldFlagBitMaskIsStaticMethod if is_static else 0 +method_info.method = func_any +method_info.metadata.data = NULL +method_info.metadata.size = 0 +CHECK_CALL(TVMFFITypeRegisterMethod(type_index, &method_info)) + +# 2. Ensure type-attr column exists (sentinel: kTVMFFINone) +CHECK_CALL(TVMFFITypeRegisterAttr(kTVMFFINone, &name_arg.cdata, &sentinel_any)) +# 3. Register as TypeAttr +CHECK_CALL(TVMFFITypeRegisterAttr(type_index, &name_arg.cdata, &func_any)) Review Comment:  The `func_any` variable, which holds an owned reference to a `FunctionObj`, is declared outside the loop and reused. This causes a reference leak in each iteration because the old object reference is overwritten without being decremented. To fix this, the `func_any` variable should be managed within the loop's scope, and its reference must be released after use. A `try...finally` block is a robust way to ensure the reference is always decremented, preventing memory leaks even if errors occur. ``` cdef TVMFFIMethodInfo method_info cdef TVMFFIAny sentinel_any sentinel_any.type_index = kTVMFFINone sentinel_any.v_int64 = 0 for name, func, is_static in py_methods: cdef TVMFFIAny func_any func_any.type_index = kTVMFFINone func_any.v_int64 = 0 cdef int c_api_ret_code try: name_bytes = c_str(name) name_arg = ByteArrayArg(name_bytes) # Convert Python callable -> TVMFFIAny (creates a FunctionObj) TVMFFIPyPyObjectToFFIAny( TVMFFIPyArgSetterFactory_, func, &func_any, &c_api_ret_code, ) CHECK_CALL(c_api_ret_code) # 1. Register as TypeMethod method_info.name = name_arg.cdata method_info.doc.data = NULL method_info.doc.size = 0 method_info.flags = kTVMFFIFieldFlagBitMaskIsStaticMethod if is_static else 0 method_info.method = func_any method_info.metadata.data = NULL method_info.metadata.size = 0 CHECK_CALL(TVMFFITypeRegisterMethod(type_index, &method_info)) # 2. Ensure type-attr column exists (sentinel: kTVMFFINone) CHECK_CALL(TVMFFITypeRegisterAttr(kTVMFFINone, &name_arg.cdata, &sentinel_any)) # 3. Register as TypeAttr CHECK_CALL(TVMFFITypeRegisterAttr(type_index, &name_arg.cdata, &func_any)) finally: if func_any.type_index >= kTVMFFIStaticObjectBegin and func_any.v_obj != NULL: TVMFFIObjectDecRef(func_any.v_obj) ``` ## python/tvm_ffi/dataclasses/py_class.py: ## @@ -202,6 +202,31 @@ def _collect_own_fields( return fields
Re: [PR] feat(python): auto-register __ffi_* dunder methods in @py_class as TypeMethod [tvm-ffi]
gemini-code-assist[bot] commented on PR #508: URL: https://github.com/apache/tvm-ffi/pull/508#issuecomment-4106713614 ## Summary of Changes Hello, I'm Gemini Code Assist[^1]! 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 interoperability between Python dataclasses and the FFI system by automating the registration of specific Python dunder methods. This allows C++ and other FFI languages to directly invoke Python-defined logic for core object operations like representation, equality, hashing, and comparison, thereby streamlining the integration of Python objects into the FFI runtime without requiring manual C++ changes. ### Highlights * **Automatic FFI Dunder Method Registration**: The `@py_class` decorator has been extended to automatically detect and register recognized FFI dunder methods (e.g., `__ffi_repr__`, `__ffi_eq__`, `__s_equal__`) defined on Python dataclasses. * **C++ Callable Methods**: Registered FFI dunder methods are now callable from C++ and other FFI languages, enabling cross-language invocation of Python-defined behaviors. * **Configurable Allowlist**: An allowlist (`_FFI_RECOGNIZED_METHODS`) has been introduced in `py_class.py` to control which specific dunder methods are collected and registered, preventing unintended exposure. * **C API Integration**: The registration process leverages the C API, specifically `TVMFFITypeRegisterMethod` and `TVMFFITypeRegisterAttr`, to integrate Python methods into the FFI type system. 🧠**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](https://codeassist.google/code-review/login) to enable Memory in your admin console.** Using Gemini Code Assist The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either `/gemini ` or `@gemini-code-assist `. Below is a summary of the supported commands on the current page. Feature | Command | Description --- | --- | --- Code Review | `/gemini review` | Performs a code review for the current pull request in its current state. Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state. Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. Help | `/gemini help` | Displays a list of available commands. 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 `.gemini/` folder in the base of the repository. Detailed instructions can be found [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). 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 :thumbsup: and :thumbsdown: 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](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG). [^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487). -- 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]
