Don't serialize DQ attrs#36875
Conversation
github-actions
Bot
added
the
category: transformations
|
Does their presence break something? @praasz does supporting these attributes in the IR bring any value? |
The attribute itself is not an issue but internal operator should not be serialized to IR. |
|
After offline discussion this change is not required and current state is OK. PR can be closed |
mitruska
left a comment
mitruska
left a comment
There was a problem hiding this comment.
My suggestion is to close this PR and keep visit_attributes as is.
It doesn't prevent DynamicQuantize op serialization anyway, just serialization of the attributes.
| // This is used for comparison during SharedOpOpt pass. | ||
| // The attrs are not a part of the official IR format, so we need to skip them during serialization. | ||
| if (dynamic_cast<ov::util::XmlSerializer*>(&visitor)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
My suggestion is to drop this change and keep visit_attributes as is.
It doesn't prevent the op serialization anyway, just serialization of the attributes.
DynamicQuantize itself (not only the attrs) is an internal operation and is not part of the public IR unless explicitly introduced by MOC or frontend transformations. Even before the mentioned PR where visit_attributes was added, the op could still be serialized, just without its attributes.
5 participants
Followup to #35283 (comment).
The attrs visited in the function are not in the offical IR spec so they shouldn't be serialized. They need to be visited for the sake of comparison performed in SharedOpOptimization pass. This pass currently doesn't apply to DQ nodes, however it will in one of the changes intended to be introduced in the near future - that's why I decided to exclude serialization instead of removing the function altogether.