-
Notifications
You must be signed in to change notification settings - Fork 64
Unify rule implementations with classes #2288
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
justinchuby
commented
May 10, 2025
•
edited
Loading
edited
- Replace RewriteRuleAsClass with RewriteRuleClassBase to unify rule implementations.
- Also: Use as ints() on attributes in rewrite rules
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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.
Pull Request Overview
This PR unifies rule implementations by replacing RewriteRuleAsClass with RewriteRuleClassBase and updates attribute handling to use as_ints() where applicable.
- Removed RewriteRuleAsClass and refactored related rule classes to use instance methods.
- Updated attribute comparisons (using as_ints()) and adjusted class member access from class attributes to instance attributes.
- Updated documentation to reference RewriteRuleClassBase.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
onnxscript/rewriter/pattern.py | Removed RewriteRuleAsClass and introduced RewriteRuleClassBase. |
onnxscript/rewriter/ort_fusions/fused_matmul_rule_sets.py | Changed rule definitions to instance methods and updated attribute access using self._pos. |
onnxscript/rewriter/llama_rule_sets.py | Refactored rule classes to use RewriteRuleClassBase and updated attribute conversions to as_ints(). |
docs/api/rewriter_pattern.md | Updated documentation to reflect the new base class usage. |
Comments suppressed due to low confidence (1)
onnxscript/rewriter/pattern.py:1724
- The removal of RewriteRuleAsClass is comprehensive. Please ensure that any legacy references or tests depending on it have been updated to use RewriteRuleClassBase.
class RewriteRuleAsClass:
Co-authored-by: Copilot <[email protected]>
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.
LGTM, just a minor question about the use of .value
vs .asint