fix(pir): a matching attribute name is not a successful read - #1655
Open
aboccag wants to merge 2 commits into
Open
fix(pir): a matching attribute name is not a successful read#1655aboccag wants to merge 2 commits into
aboccag wants to merge 2 commits into
Conversation
Paddle 3.3 stores some float-typed array attributes (notably the values of
full_int_array) as Int64Attribute, but the parser asserted the first element
was a FloatAttribute and aborted with
the 0th elementwise MUST be ir::FloatAttribute
Accept Int32/Int64/Double as well and cast to float.
GetOpAttr set `found` as soon as an attribute's *name* matched, before
looking at its type. When the type was one the overload does not read, the
caller's variable was left exactly as it was -- uninitialised, for the bare
`float alpha_;` members mappers declare -- and that garbage went into the
ONNX graph as if it were the model's own value.
pd_op.leaky_relu is the case that surfaced it. PIR stores its negative_slope
as a double; LeakyReluMapper asks for a float; the isa<FloatAttribute> test
failed, nothing was written, and every exported LeakyRelu got
alpha = 2.466285297211678e-43
instead of 0.1 -- a denormal, so leaky_relu became a plain ReLU. The models
converted, loaded and ran; they were simply wrong. On yolov3 the whole neck
diverged (max|diff| 3.8 at the first neck output, against 3.8e-06 through
the entire backbone) and the detector returned one box where Paddle returned
98.
So: read a double where a float is asked for and vice versa, since PIR
stores as a double several attributes the legacy IR stored as a float; and
make a type that cannot be read fail as loudly as a missing attribute,
through the whole family of overloads rather than just this one.
That strictness immediately caught a second instance: FullIntArrayMapper
read `dtype` into a std::string while the attribute is a DataTypeAttribute.
It never used the value -- Opset7 takes the type from the output -- so the
read is simply dropped.
yolov3_mobilenet_v1_270e_coco now matches Paddle inference over 50 COCO
images: 136 boxes, worst IoU 0.999994, worst score delta 2.3e-06.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PaddlePirParser::GetOpAttrsets itsfoundflag as soon as an attribute'sname matches, before looking at its type. When the type is one the overload
does not read, the caller's variable is left exactly as it was — uninitialised,
for the bare
float alpha_;members mappers declare — and the enforce belowstill passes, so nothing is reported.
pd_op.leaky_reluis the case that surfaced it. PIR stores itsnegative_slopeas a double;
LeakyReluMapperasks for a float; theisa<FloatAttribute>()test fails, nothing is written, and every exportedLeakyRelu gets
That is a denormal — whatever was on the stack — so
leaky_relubecomes a plainReLU. The models convert, load and run; they are simply wrong. Onyolov3_mobilenet_v1_270e_cocothe whole neck diverges (max|diff| 3.8 at thefirst neck output, against 3.8e-06 through the entire backbone) and the detector
returns one box where Paddle returns 98.
What this changes
doublewhere afloatis asked for and vice versa. PIR stores as adouble several attributes the legacy IR stored as a float.
GetOpAttr<std::vector<float>>: Paddle 3.3stores some float-typed array attributes (
full_int_array's values, for one)as integers.
across the whole family of overloads rather than just the one.
That last point is a behaviour change, so it was measured rather than assumed:
across all 149 PaddleDetection detection configs, zero status changes —
every model that exported and converted before still does, model by model.
It also immediately caught a second instance:
FullIntArrayMapperreaddtypeinto a
std::stringwhile the attribute is aDataTypeAttribute. It never usedthe value —
Opset7takes the type from the output — so the read is dropped.Verification
yolov3_mobilenet_v1_270e_cocoafter the fix, against Paddle inference over 50COCO images: 136 boxes compared, worst pairwise IoU 0.999994, worst score delta
2.3e-06.
The fix also repaired three models that had been failing numerically for
unrelated-looking reasons —
picodet_shufflenetv2_1x_416_coco,centernet_shufflenetv2_140e_cocoandttfnet_darknet53_1x_coco, whose graphscontain 36, 36 and 52 LeakyRelu nodes respectively.
ttfnet_darknet53had beenreturning no boxes at all.