Add ONNX parsing for SkipSimplifiedLayerNormalization #3140
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3140 +/- ##
========================================
Coverage 92.25% 92.25%
========================================
Files 500 501 +1
Lines 20054 20087 +33
========================================
+ Hits 18500 18531 +31
- Misses 1554 1556 +2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| // bias (optional) : T | ||
| // 1D bias tensor with shape (hidden_size) - not used by ORT | ||
|
|
||
| if(args.size() != 3) |
There was a problem hiding this comment.
Is there not a 4th input to handle then? Based on the doc its specifying 3-4 inputs.
There was a problem hiding this comment.
Onnxruntime didn't implement theirs to utilize the bias input, possibly because the only model that currently uses this op doesn't call it with a bias input. I could add it if we want to be inline with the specs, but since I was using theirs as a baseline for correctness I wrote ours the same way.
There was a problem hiding this comment.
Yeah I get it. Nothing wrong with what you did. I think its better to cover our bases on this though as we've seen fun outcomes with some of these optional inputs
My only concern really is because microsoft wrote:
This version of the operator has been available since version 1 of the 'com.microsoft' operator set.
TedThemistokleous
added
the
Onnx Operators
| std::vector<half> x{half{0.8}, | ||
| half{-0.5}, | ||
| half{0.0}, | ||
| half{1.0}, | ||
| half{0.5}, | ||
| half{0.2}, | ||
| half{0.3}, | ||
| half{-0.6}, | ||
| half{10.0}, | ||
| half{-1.0}, | ||
| half{0.0}, | ||
| half{1.0}, | ||
| half{1.2}, | ||
| half{3.2}, | ||
| half{-4.1}, | ||
| half{5.3}}; |
There was a problem hiding this comment.
Removing the casts causes compilation errors. It looks like the way I have it is what we do in other tests using the half type.
There was a problem hiding this comment.
It does look like we use it like this, but just as an alternate solution you can try passing Float vector to migraphx::argument which has "half_type" as its shape.
|
|
||
| if(x_rank < 2 or x_rank > 3 or x_rank != skip_rank or gamma_rank != 1) | ||
| { | ||
| MIGRAPHX_THROW("PARSE_SKIPSIMPLIFIEDLAYERNORMALIZATION: invalid input shape"); |
There was a problem hiding this comment.
Add some test cases for the invalid input cases that trigger this and the above throws
|
@turneram fix tidy |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
No description provided.