feat(python): add Function.__init__ constructor for direct callable wrapping#492
feat(python): add Function.__init__ constructor for direct callable wrapping#492junrushao wants to merge 1 commit intoapache:mainfrom
Conversation
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 enhances the 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a convenient __init__ constructor for tvm_ffi.Function, allowing direct instantiation from callables. The implementation correctly handles both new callables and copy-construction from existing Function objects, including checks for moved-from instances. The accompanying tests are thorough and cover the main use cases and error conditions. I've found a minor memory leak in one of the new tests and suggested a fix. Overall, this is a great addition that improves the usability of the FFI.
749bb17 to
1ff8b85
Compare
…rapping Architecture: - Adds `__init__` to the Cython `Function` extension type in `function.pxi`. For plain callables, delegates to the existing `_convert_to_ffi_func_handle` helper. For `Function`-to-`Function` copies, increments the C-level ref count via `TVMFFIObjectIncRef` and shares the handle. - `func` parameter defaults to `None` for backward compatibility: when omitted the object remains in its default null-handle state (matching the prior behavior where `__cinit__` left `chandle = NULL`). - No changes to the C ABI, C++ layer, or global registry. Public Interfaces: - New: `tvm_ffi.Function(func=None)` constructor accepting any Python callable, an existing `Function`, or `None` (default, null handle). - Type stub updated in `core.pyi` with `Callable[..., Any] | None = None`. UI/UX: - none Behavioral Changes: - Previously `tvm_ffi.Function` could only be obtained via `tvm_ffi.convert()` or the global function registry. Now it can be constructed directly. - Copy-constructing from a `Function` produces a second ref to the same underlying packed function (verified via `same_as`). - Calling `Function()` with no arguments preserves backward compatibility by returning a null-handle instance identical to the prior default construction. Docs: - Cython docstring and `.pyi` stub updated. No standalone doc page exists for `Function.__init__`; the stub is the primary surface. Tests: - Executed: `uv run pytest -vvs tests/python/test_function.py::test_pyfunc_init` - Result: PASSED (covers callable wrapping, copy-construct, moved-from ValueError, no-arg null-handle backward compat, non-callable TypeError) - Fixed test handle leak: moved-from test now uses `__new__` to avoid allocating a handle that gets immediately overwritten by `__move_handle_from__`. Untested Edge Cases: - Thread-safety of concurrent `__init__` calls (covered by GIL in CPython). - Interaction with `release_gil` property on newly constructed function (inherits default from `__cinit__`).
1ff8b85 to
ec4ab4c
Compare
| def __cinit__(self) -> None: | ||
| self.c_release_gil = _RELEASE_GIL_BY_DEFAULT | ||
|
|
||
| def __init__(self, func: Optional[Callable[..., Any]] = None) -> None: |
There was a problem hiding this comment.
let us wait until cutedsl release cycles fixes the compact issue, the land this one
Summary
Function.__init__constructor to the CythonFunctionclass, enabling direct construction viatvm_ffi.Function(callable)instead of requiringtvm_ffi.convert(callable)Function(with ref-count increment), or construct with no arguments for backward compatibility (null handle)TypeErrorand moved-fromFunctionobjects withValueErrorArchitecture
__init__to the CythonFunctionextension type infunction.pxi. For plain callables, delegates to the existing_convert_to_ffi_func_handlehelper. ForFunction-to-Functioncopies, increments the C-level ref count viaTVMFFIObjectIncRefand shares the handle.funcparameter defaults toNonefor backward compatibility: when omitted the object remains in its default null-handle state (matching the prior behavior where__cinit__leftchandle = NULL).Public Interfaces
tvm_ffi.Function(func: Callable[..., Any] | None = None) -> Functionconstructor.core.pyi.Behavioral Changes
tvm_ffi.Functioncould only be obtained viatvm_ffi.convert()or the global function registry. Now it can be constructed directly.Functionproduces a second ref to the same underlying packed function (verified viasame_as).Function()with no arguments preserves backward compatibility by returning a null-handle instance identical to the prior default construction.Tests
test_pyfunc_initcovers: wrapping a Python callable, copy-constructing fromFunction,ValueErroron moved-from source, no-arg null-handle backward compat,TypeErroron non-callable input.__new__to avoid leaking a handle that would be immediately overwritten by__move_handle_from__.Untested Edge Cases
__init__calls (covered by GIL in CPython).release_gilproperty on the newly constructed function (inherits default from__cinit__).