-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[TE] [FFI] Fix broken axis/reduce_axis properties in BaseComputeOp and ScanOp after FFI refactoring #18375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @BenkangPeng, 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 addresses a critical issue introduced during the TVM FFI refactoring, which rendered several key properties ( Highlights
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly fixes a bug where several attributes on BaseComputeOp and ScanOp were inaccessible from Python after an FFI refactoring. The fix involves removing the old, broken @property wrappers, which is the right approach. My review includes suggestions to add docstrings to the affected classes. This will improve code maintainability by explaining that these attributes are now automatically exposed by the FFI, a detail mentioned in the pull request description but missing from the code changes.
| class BaseComputeOp(Operation): | ||
| """Compute operation.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While removing the broken properties is correct, it would be beneficial for code readability and maintainability to add a docstring explaining that axis and reduce_axis are automatically exposed by the FFI, as you mentioned in the PR description. This helps future developers understand where these attributes come from without having to trace back to the C++ FFI registration.
How about updating the class docstring?
| class BaseComputeOp(Operation): | |
| """Compute operation.""" | |
| class BaseComputeOp(Operation): | |
| """Compute operation. | |
| The `axis` and `reduce_axis` attributes are automatically exposed by the FFI. | |
| """ |
| class ScanOp(Operation): | ||
| """Scan operation.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to BaseComputeOp, it would be helpful to add a docstring to ScanOp to clarify that the scan_axis attribute is exposed via the FFI. This improves discoverability and maintainability.
| class ScanOp(Operation): | |
| """Scan operation.""" | |
| class ScanOp(Operation): | |
| """Scan operation. | |
| The `scan_axis` attribute is automatically exposed by the FFI. | |
| """ |
Summary
This PR fixes a bug introduced during the TVM FFI refactoring where
BaseComputeOp.axis,BaseComputeOp.reduce_axis, andScanOp.scan_axisattributes became inaccessible from Python.Problem
After the TVM FFI library was separated to
3rdparty/tvm-ffi, the Python property wrappersin
python/tvm/te/tensor.pystopped working because they rely on__getattr__, which nolonger exists in the new FFI system.
Error when accessing these attributes:
Root Cause
In C++ (
include/tvm/te/operation.hlines 147-149),BaseComputeOpNode's attributes are properlyregistered via FFI reflection:
refl::ObjectDef<BaseComputeOpNode>() .def_ro("axis", &BaseComputeOpNode::axis) .def_ro("reduce_axis", &BaseComputeOpNode::reduce_axis);The new FFI system (
3rdparty/tvm-ffi/python/tvm_ffi/registry.py) automaticallycreates Python properties for these C++ attributes.
However, in
python/tvm/te/tensor.py, the old@propertywrappers usingself.__getattr__("axis")block the FFI auto-generated properties from being added:The FFI registry skips adding properties if they already exist on the class
``3rdparty/tvm-ffi/python/tvm_ffi/registry.py` lines 272-275
Solution
Remove the broken Python property wrappers in
BaseComputeOpandScanOp, allowingthe FFI system to automatically expose these C++ attributes as intended.
The FFI-generated properties provide the same functionality without requiring manual
wrapper code.
Changes
python/tvm/te/tensor.py:@propertydecorators foraxisandreduce_axisinBaseComputeOp@propertydecorator forscan_axisinScanOp.Testing
Verified that the attributes are now accessible: