-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[BREAKING CHANGE] Prefix all type keys and function names with "tvm." #18611
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
base: main
Are you sure you want to change the base?
[BREAKING CHANGE] Prefix all type keys and function names with "tvm." #18611
Conversation
d5d2923 to
add3ba5
Compare
Summary of ChangesHello @junrushao, 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 implements a comprehensive breaking change by standardizing the naming convention for all FFI-exposed type keys and global function names within the TVM project. By consistently adding a 'tvm.' prefix, the change aims to improve clarity, prevent potential naming collisions, and establish a well-defined namespace for TVM's internal and external API components across C++ and Python interfaces. This is a foundational change affecting numerous core components and bindings. 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. 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 introduces a breaking change by prefixing all type keys and function names with tvm. to align with stubgen conventions. The changes are extensive and cover a wide range of files across the codebase. While the majority of the changes appear to be correct and consistent with the stated goal, I've identified a few critical issues, particularly in the Relax parser and testing utilities, where the automated replacement seems to have introduced bugs that could break functionality. Additionally, there are several instances of redundant naming (e.g., tvm.tvm_...) that could be simplified for better clarity and consistency. My review includes suggestions to fix these issues.
| "Tensor", | ||
| "Tuple", | ||
| "function", | ||
| "tvm.function", |
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.
| # look for the named argument in the function decorator | ||
| for dec in node.decorator_list: | ||
| if not isinstance(dec, doc.Call) or dec.func.attr != "function": | ||
| if not isinstance(dec, doc.Call) or dec.func.attr != "tvm.function": |
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.
The decorator name is function, not tvm.function. The dec.func.attr should be compared against 'function'. This change will break parsing of @R.function decorators.
| if not isinstance(dec, doc.Call) or dec.func.attr != "tvm.function": | |
| if not isinstance(dec, doc.Call) or dec.func.attr != "function": |
| # many times the fixture has been used. If used, the cache gets | ||
| # cleared after the fixture is no longer needed. | ||
| scope = "function" | ||
| scope = "tvm.function" |
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.
|
|
||
| # override metal compiler to compile to iphone | ||
| @tvm.register_global_func("tvm_callback_metal_compile") | ||
| @tvm.register_global_func("tvm.tvm_callback_metal_compile") |
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.
The new function name tvm.tvm_callback_metal_compile seems redundant. To improve clarity and consistency with the new naming scheme, consider renaming it to tvm.callback_metal_compile. This would make the naming scheme more consistent by avoiding the repeated tvm prefix.
| @tvm.register_global_func("tvm.tvm_callback_metal_compile") | |
| @tvm.register_global_func("tvm.callback_metal_compile") |
| @tvm_ffi.register_global_func("tvm.tvm_callback_cuda_compile") | ||
| def tvm_callback_cuda_compile(code, target): # pylint: disable=unused-argument |
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.
The new function name tvm.tvm_callback_cuda_compile seems redundant. To improve clarity and consistency, consider renaming it to tvm.callback_cuda_compile and also renaming the Python function to callback_cuda_compile.
| @tvm_ffi.register_global_func("tvm.tvm_callback_cuda_compile") | |
| def tvm_callback_cuda_compile(code, target): # pylint: disable=unused-argument | |
| @tvm_ffi.register_global_func("tvm.callback_cuda_compile") | |
| def callback_cuda_compile(code, target): # pylint: disable=unused-argument |
|
|
||
|
|
||
| @tvm_ffi.register_global_func("tvm_callback_libdevice_path") | ||
| @tvm_ffi.register_global_func("tvm.tvm_callback_libdevice_path") |
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.
|
|
||
|
|
||
| @tvm_ffi.register_global_func("tvm_callback_rocm_link") | ||
| @tvm_ffi.register_global_func("tvm.tvm_callback_rocm_link") |
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.
|
|
||
|
|
||
| @tvm_ffi.register_global_func("tvm_callback_rocm_bitcode_path") | ||
| @tvm_ffi.register_global_func("tvm.tvm_callback_rocm_bitcode_path") |
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.
|
|
||
|
|
||
| @tvm_ffi.register_global_func("tvm_callback_rocm_get_arch") | ||
| @tvm_ffi.register_global_func("tvm.tvm_callback_rocm_get_arch") |
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.
add3ba5 to
d480bc9
Compare
To be compatible with stubgen convention