[Draft] To proto move attributes #22159
Conversation
fix handling of multiple QuantizeLinear nodes
fix debug build
* Update onnx_ctx_model_helper.cc Keep "attr_0" "attr_1" etc inside a scope so that these unique pointers get deleted after their use is over Also declare "node_attributes" global to the new scope (within the function) * Optimizations to reduce memory used during compilation * Fix for reusing the serialized compiled blob to proto model * EpCtx export code refactoring * Fix from Ankit to move memory during ONNX Graph creation * Localise the OV IR model in memory * Apply lint changes --------- Co-authored-by: Vishnudas Thaniel S <[email protected]> Co-authored-by: Javier E. Martinez <[email protected]>
Updated the Version of OV to 2024.3
update: Enable C++ 20 Standard for the build
| template <bool copy_attributes = true> | ||
| static void ToProtoInternal(ONNX_NAMESPACE::NodeProto& proto, bool update_subgraphs = false); |
There was a problem hiding this comment.
nit: templatizing like this typically results in unnecessary binary size cost vs. simply passing a boolean argument. given there is only one place with constexpr (copy_attributes) there doesn't seem to be a significant benefit.
| @@ -475,6 +475,10 @@ class Node { | |||
| to ensure the complete Graph is valid. | |||
| */ | |||
| void ToProto(ONNX_NAMESPACE::NodeProto& proto, bool update_subgraphs = false) const; | |||
| void ToProtoFinal(ONNX_NAMESPACE::NodeProto& proto, bool update_subgraphs = false); | |||
There was a problem hiding this comment.
nit: document public members. would be good to explain when/why you should use ToProto vs ToProtoFinal.
| @@ -183,6 +183,7 @@ class Model { | |||
| #if !defined(ORT_MINIMAL_BUILD) | |||
| // Get model's serialization proto data. | |||
| ONNX_NAMESPACE::ModelProto ToProto() const; | |||
| ONNX_NAMESPACE::ModelProto ToProtoFinal(); | |||
| instance.graph_proto_->clear_node(); | ||
| instance.graph_proto_->clear_input(); | ||
| instance.graph_proto_->clear_output(); | ||
| instance.graph_proto_->clear_value_info(); |
There was a problem hiding this comment.
Existing code, but it's a little odd to be modifying graph_proto_ in what was a const member function. Should this be modifying instance.graph_proto_ at all if the idea is to populate graph_proto?
| template <typename TInstance> | ||
| void Graph::ToGraphProtoInternal(TInstance& instance, ONNX_NAMESPACE::GraphProto& graph_proto) { |
There was a problem hiding this comment.
Would be good to avoid templatizing here as well.
One option would be to pass in std::variant<Graph*, const Graph*>. Use the mutable one if present and call ToProtoFinal.
Another would be to pass in a functor to do the node update. It could capture the const/non-const Graph instance and call Node::ToProto[Final] as applicable.
Description
Add changes to support moving attributes when generating proto from an EpContext graph for the purpose of saving to disk.
Motivation and Context
The changes aim to eliminate a compiled model copy in memory in the process of creating and saving an EpContext graph to disk.