-
Notifications
You must be signed in to change notification settings - Fork 49
[CYTHON][REFACTOR] __tvm_ffi_object__ protocol #115
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 @tqchen, 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 refactors the internal object caching mechanism and generalizes the FFI protocol within the 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 refactors the FFI object protocol by renaming PyNativeObject.__tvm_ffi_object__ to _tvm_ffi_cached_object and generalizing the __tvm_ffi_tensor__ protocol to __tvm_ffi_object__. The changes are mostly consistent and well-applied across the codebase. I've found a potential issue where a type index is hardcoded, which contradicts the goal of generalizing the protocol. I've also noted a minor docstring error. Overall, a good refactoring effort.
This PR does two things: - We renamed PyNativeObject.__tvm_ffi_object__ to _tvm_ffi_cached_object This is because the dunder convention is normally used for functions instead of attributes - We then updated the original __tvm_ffi_tensor__ protocol to __tvm_ffi_object__ so that the same protocol can be used for generic objects. Given the original attribute is hidden to user and we did not yet introduce __tvm_ffi_tensor__ in a formal release, the change should have low impact. We will however freeze the protocol for formal release ideally soon.
5600220 to
fe5464b
Compare
This PR does two things:
PyNativeObject.__tvm_ffi_object__to_tvm_ffi_cached_objectThis is because the dunder convention is normally used for functions instead of attributes__tvm_ffi_tensor__protocol to__tvm_ffi_object__so that the same protocol can be used for generic objects.Given the original attribute is hidden to user and we did not yet introduce
__tvm_ffi_tensor__in a formal release, the change should have low impact. We will however freeze the protocol for formal release ideally soon.