-
Notifications
You must be signed in to change notification settings - Fork 83
fix: handling of default attrs in SimplifiedLayerNormalization + LayerNormalization🐛 #2396
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
❌ 5 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 fixes how default attributes (epsilon
, stash_type
) are handled in both LayerNormalization
and SimplifiedLayerNormalization
fusions, adds a BART encoder model to the fusion tests, and introduces commuted-input support for SkipLayerNormalization
rules.
- Extract default
epsilon
from the matched node instead of requiring it in the pattern signature - Add
test_bart_encoder
to validate fusion with default-attribute cases - Enable commuted-input variants by applying
.commute()
to fusion rules
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
skip_normalization_test.py | Added test_bart_encoder to cover default-attribute fusions |
skip_normalization.py | Refactored patterns to drop default attrs, extract epsilon in rewrite, and apply rule commutation |
Comments suppressed due to low confidence (2)
onnxscript/rewriter/ort_fusions/skip_normalization_test.py:73
- The test uses
fuse_skip_layer_normalization(model)
but there is no import for that symbol in this file. Please addfrom onnxscript.rewriter.ort_fusions.skip_normalization import fuse_skip_layer_normalization
(or adjust the import path) to ensure the function is available.
fuse_skip_layer_normalization(model)
onnxscript/rewriter/ort_fusions/skip_normalization.py:231
- The new
.commute()
calls are applied only to the fullSkipLayerNormalization
rules. To allow commuted inputs forSkipSimplifiedLayerNormalization
as well, you should apply.commute()
to the simplified-layer ruleset (if defined) or include those here before applyingapply_fusion_rules
.
skip_layer_normalization_ruleset = pattern.RewriteRuleSet(
Looks like there are some comments unaddressed |
@justinchuby Sorry, was busy at work. Resolved one issue, for the other I'd need @gramalingam s input. 👍 |
@KarelZe feel free to take a look at the final round of comments. After that I think we are ready to merge. Thanks! |
@justinchuby Should be complete now🙌 Just one more question, would you be open to /interested in adding |
Thanks! Would let @gramalingam comment |
Sure, that would be great, thanks! |
SkipLayerNormFusion
does currently not fuse ops, if stash_type is at default (=1) or epsilon is at default (=1e-5) forLayerNormalization
andSimplifiedLayerNormalization
This pr:
LayerNormalization
,SimplifiedLayerNormalization
EmbedLayerNormalization
.Closes #2378.
@shubhambhokare1 @justinchuby Could you please review? Any feedback is greatly appreciated.