[TIR] improve typing for tir.PrimExpr#18640
Conversation
Summary of ChangesHello @clouds56, 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 significantly improves the type annotations within 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. 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
The pull request refines type hints in the ExprOp class within python/tvm/tir/expr.py by broadening the other argument for various operator overload methods (e.g., __add__, __mul__, __div__, comparison, and bitwise operators) to accept numeric types (int, float, complex) or int/bool in addition to PrimExpr. It also makes the return types of these methods more specific, using string literals like "Add", "Mul", "Call", and updates the astype method's return type to Union["Cast", "Self"]. A new numeric type alias was introduced, and Self was imported. A review comment suggests further updating the type hints for __xor__ and __rxor__ to include bool as an accepted type for the other argument, ensuring consistency with __and__ and __or__ given Python's behavior for bitwise operations on booleans.
|
The lint is false positive in this case I update pylint from 3.0.0 to 3.3.9 the problem disappears. |
|
@clouds56 Thanks for working on this, and sorry for the late reply. After looking through the full change, I don’t think we should have it in the current form. I’d rather say that clearly now than keep dragging the review out. Main concerns:
TVM CI only enforces ruff today; we do not enforce mypy or pyright anywhere in the project. That means adding several new public aliases plus a large number of signature changes increases our maintenance burden, without any mechanism that ensures those annotations remain correct over time. In practice, we would be taking on long-term annotation debt for limited in-tree benefit.
In TIR,
For example, annotating
The motivating issue is real: IDE squiggles on common patterns like Thanks again for the effort here. I don’t think the overall motivation is wrong, but the current scope and the interaction with TIR’s dtype/semantic discipline make this hard to merge as-is. |
|
Thanks for the detailed feedback. I’m still relatively new to TVM and am actively learning the primitives and APIs, so please feel free to correct me if I misunderstand anything. annotation in the repo should be more like a
|
|
Thanks for the pushback, @clouds56! I want to revise part of my earlier reply. I agree that the input-side widening ( What I still do not agree with:
More broadly, I do not think this PR should be treated as approval for the larger direction. Extending the same pattern to more APIs, upgrading pylint expectations, or including unrelated typing cleanups like Whether it still makes sense to push this kind of change — even scoped down to input widening + expression aliases — probably deserves more discussion before we commit to a direction. cc @tqchen — would appreciate your opinion. Specifically: given that we do not currently enforce mypy / pyright in CI, is it worth introducing |
Motivation
Now we use PrimExpr as type hint in most places and it would report errors (in static typing) sometimes.
case 1:
case 2:
Solution
This PR is to mitigate these cases by introducing
Problems
ForFramedoesn't have__iter__, this would be fixed in another PR. Current PR would focus onPrimExpr. POC here 7d64de1. It could be included in this PR if you like.