From bcd50afafbb95d9206535802669a7b4a9ec5a019 Mon Sep 17 00:00:00 2001 From: Guoyu Wang <62914304+gwang-msft@users.noreply.github.com> Date: Thu, 15 Jul 2021 11:52:01 -0700 Subject: [PATCH] [CoreML EP] Fix failure for layer without name (#8399) * Fix CoreML failure for layer without name * Update clip op builder to new naming function --- .../builders/impl/activation_op_builder.cc | 2 +- .../coreml/builders/impl/argmax_op_builder.cc | 4 ++-- .../coreml/builders/impl/base_op_builder.cc | 19 +++++++++++++++---- .../coreml/builders/impl/base_op_builder.h | 5 ++++- .../builders/impl/batch_norm_op_builder.cc | 2 +- .../coreml/builders/impl/binary_op_builder.cc | 2 +- .../coreml/builders/impl/clip_op_builder.cc | 15 ++++++++++----- .../coreml/builders/impl/concat_op_builder.cc | 2 +- .../coreml/builders/impl/conv_op_builder.cc | 2 +- .../coreml/builders/impl/gemm_op_builder.cc | 2 +- .../coreml/builders/impl/pool_op_builder.cc | 2 +- .../builders/impl/reshape_op_builder.cc | 2 +- .../coreml/builders/impl/resize_op_builder.cc | 2 +- .../builders/impl/squeeze_op_builder.cc | 2 +- .../builders/impl/transpose_op_builder.cc | 2 +- 15 files changed, 42 insertions(+), 23 deletions(-) diff --git a/onnxruntime/core/providers/coreml/builders/impl/activation_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/activation_op_builder.cc index f5d13c64bc..7b4001139b 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/activation_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/activation_op_builder.cc @@ -25,7 +25,7 @@ class ActivationOpBuilder : public BaseOpBuilder { Status ActivationOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& /* logger */) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); const auto& op_type(node.OpType()); if (op_type == "Sigmoid") { diff --git a/onnxruntime/core/providers/coreml/builders/impl/argmax_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/argmax_op_builder.cc index e5b0d015e3..b9d51ef856 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/argmax_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/argmax_op_builder.cc @@ -27,7 +27,7 @@ class ArgMaxOpBuilder : public BaseOpBuilder { Status ArgMaxOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& /* logger */) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); const auto& graph_viewer = model_builder.GetGraphViewer(); NodeAttrHelper helper(node); @@ -46,7 +46,7 @@ Status ArgMaxOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, if (node.GetOutputEdgesCount() == 1) { auto it = node.OutputEdgesBegin(); const auto* succ_node(graph_viewer.GetNode(it->GetNode().Index())); - // If Argmax's successive node is a Cast from int64 to int32 output + // If Argmax's successive node is a Cast from int64 to int32 output // The 'cast to' type is checked in operater supported related, omit the check here if (succ_node->OpType() == "Cast") { // Skip the cast's input/argmax's output diff --git a/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.cc index 4c3b5333d6..298d6342e6 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.cc @@ -50,10 +50,21 @@ Status BaseOpBuilder::AddToModelBuilder(ModelBuilder& model_builder, const Node& return Status::OK(); } -/* static */ std::unique_ptr BaseOpBuilder::CreateNNLayer(const Node& node) { - std::unique_ptr layer = - std::make_unique(); - layer->set_name(node.Name()); +/* static */ std::unique_ptr +BaseOpBuilder::CreateNNLayer(ModelBuilder& model_builder, const Node& node) { + auto layer_name = node.Name(); + if (layer_name.empty()) { + // CoreML requires layer has a name, while the node name is optional in ONNX + // In this case, create a unique name for the layer + layer_name = model_builder.GetUniqueName(MakeString("Node_", node.Index(), "_type_", node.OpType())); + } + return CreateNNLayer(layer_name); +} + +/* static */ std::unique_ptr +BaseOpBuilder::CreateNNLayer(const std::string& layer_name) { + std::unique_ptr layer = std::make_unique(); + layer->set_name(layer_name); return layer; } diff --git a/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.h b/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.h index f4b01418d6..05043a566f 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.h +++ b/onnxruntime/core/providers/coreml/builders/impl/base_op_builder.h @@ -24,7 +24,10 @@ class BaseOpBuilder : public IOpBuilder { virtual Status AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& logger) const ORT_MUST_USE_RESULT = 0; - static std::unique_ptr CreateNNLayer(const Node& node); + static std::unique_ptr + CreateNNLayer(ModelBuilder& model_builder, const Node& node); + + static std::unique_ptr CreateNNLayer(const std::string& layer_name); // Operator support related public: diff --git a/onnxruntime/core/providers/coreml/builders/impl/batch_norm_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/batch_norm_op_builder.cc index 63f750cd0f..973c77e5ec 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/batch_norm_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/batch_norm_op_builder.cc @@ -45,7 +45,7 @@ void BatchNormalizationOpBuilder::AddInitializersToSkip(ModelBuilder& model_buil Status BatchNormalizationOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& /* logger */) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); const auto& input_defs = node.InputDefs(); const auto& initializers(model_builder.GetInitializerTensors()); diff --git a/onnxruntime/core/providers/coreml/builders/impl/binary_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/binary_op_builder.cc index 46a06301bb..3af1e82122 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/binary_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/binary_op_builder.cc @@ -30,7 +30,7 @@ Status BinaryOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const const auto& op_type(node.OpType()); const auto& input_defs(node.InputDefs()); - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); if (op_type == "Add") { layer->mutable_add(); diff --git a/onnxruntime/core/providers/coreml/builders/impl/clip_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/clip_op_builder.cc index 3bf23dcc6f..4f83ba66d5 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/clip_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/clip_op_builder.cc @@ -53,7 +53,7 @@ Status ClipOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, if (!has_min && !has_max) { // Clip without min/max is an identity node // In CoreML we don't have identity, use ActivationLinear instead - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); layer->mutable_activation()->mutable_linear()->set_alpha(1.0f); *layer->mutable_input()->Add() = input_name; *layer->mutable_output()->Add() = output_name; @@ -78,7 +78,8 @@ Status ClipOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, // Handle clipping at min first if (has_min) { - std::unique_ptr min_layer = CreateNNLayer(node); + const auto clip_min_layer_name = model_builder.GetUniqueName(MakeString(node_name, "_Clip_min")); + std::unique_ptr min_layer = CreateNNLayer(clip_min_layer_name); if (min == 0.0f) { // If min is 0. then this min will be handled by relu min_layer->mutable_activation()->mutable_relu(); } else { // otherwise, min will be handled by unary->threshold @@ -93,9 +94,11 @@ Status ClipOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, // Clipping at max is handled by -1 * (threshold (-min_output, -max)) if (has_max) { - const auto threshold_output_name = model_builder.GetUniqueName(node_name + "threshold_output"); + const auto threshold_output_name = model_builder.GetUniqueName(MakeString(node_name, "threshold_output")); { // Add threshold layer, which is actually max( -1 * min_output, -max) - std::unique_ptr threshold_layer = CreateNNLayer(node); + const auto clip_max_threshold_layer_name = + model_builder.GetUniqueName(MakeString(node_name, "_Clip_max_threshold")); + auto threshold_layer = CreateNNLayer(clip_max_threshold_layer_name); threshold_layer->mutable_unary()->set_alpha(-max); threshold_layer->mutable_unary()->set_scale(-1.0f); threshold_layer->mutable_unary()->set_type(COREML_SPEC::UnaryFunctionLayerParams::THRESHOLD); @@ -104,7 +107,9 @@ Status ClipOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, model_builder.AddLayer(std::move(threshold_layer)); } { // Add linear activation layer -1 * threshold_output - std::unique_ptr linear_layer = CreateNNLayer(node); + const auto clip_max_linear_layer_name = + model_builder.GetUniqueName(MakeString(node_name, "_Clip_max_linear")); + auto linear_layer = CreateNNLayer(clip_max_linear_layer_name); linear_layer->mutable_activation()->mutable_linear()->set_alpha(-1.0f); *linear_layer->mutable_input()->Add() = threshold_output_name; *linear_layer->mutable_output()->Add() = output_name; diff --git a/onnxruntime/core/providers/coreml/builders/impl/concat_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/concat_op_builder.cc index 4f7cb63eae..d29d5ae0c4 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/concat_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/concat_op_builder.cc @@ -29,7 +29,7 @@ class ConcatOpBuilder : public BaseOpBuilder { Status ConcatOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& logger) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); layer->mutable_concat()->set_sequenceconcat(false); diff --git a/onnxruntime/core/providers/coreml/builders/impl/conv_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/conv_op_builder.cc index a6310d3282..43fc93eda3 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/conv_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/conv_op_builder.cc @@ -43,7 +43,7 @@ void ConvOpBuilder::AddInitializersToSkip(ModelBuilder& model_builder, const Nod Status ConvOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& logger) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); const auto& input_defs = node.InputDefs(); diff --git a/onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc index 52a837eb81..d69743010c 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc @@ -61,7 +61,7 @@ static std::vector GetTensorFloatDataTransposed(const ONNX_NAMESPACE::Ten Status GemmOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& /* logger */) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); const auto& op_type = node.OpType(); const auto& input_defs = node.InputDefs(); diff --git a/onnxruntime/core/providers/coreml/builders/impl/pool_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/pool_op_builder.cc index 60052c9a0d..b02b8434b9 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/pool_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/pool_op_builder.cc @@ -30,7 +30,7 @@ class PoolOpBuilder : public BaseOpBuilder { Status PoolOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& logger) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); auto* coreml_pool = layer->mutable_pooling(); const auto& op_type = node.OpType(); diff --git a/onnxruntime/core/providers/coreml/builders/impl/reshape_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/reshape_op_builder.cc index 0d737bd429..a3baf79fe0 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/reshape_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/reshape_op_builder.cc @@ -41,7 +41,7 @@ void ReshapeOpBuilder::AddInitializersToSkip(ModelBuilder& model_builder, const Status ReshapeOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& logger) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); const auto& input_defs = node.InputDefs(); const auto& initializers(model_builder.GetInitializerTensors()); diff --git a/onnxruntime/core/providers/coreml/builders/impl/resize_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/resize_op_builder.cc index c2ebb33de6..7bde293ad8 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/resize_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/resize_op_builder.cc @@ -83,7 +83,7 @@ void ResizeOpBuilder::AddInitializersToSkip(ModelBuilder& model_builder, const N Status ResizeOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& logger) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); auto* coreml_upsample = layer->mutable_upsample(); NodeAttrHelper helper(node); diff --git a/onnxruntime/core/providers/coreml/builders/impl/squeeze_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/squeeze_op_builder.cc index 3da53f4aa1..6e69ce6735 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/squeeze_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/squeeze_op_builder.cc @@ -61,7 +61,7 @@ void SqueezeOpBuilder::AddInitializersToSkip(ModelBuilder& model_builder, const Status SqueezeOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& /* logger */) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); auto* coreml_squeeze = layer->mutable_squeeze(); std::vector axes = GetAxes(model_builder, node); diff --git a/onnxruntime/core/providers/coreml/builders/impl/transpose_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/transpose_op_builder.cc index cb1e856bc7..4208988c3d 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/transpose_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/transpose_op_builder.cc @@ -23,7 +23,7 @@ class TransposeOpBuilder : public BaseOpBuilder { Status TransposeOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const Node& node, const logging::Logger& logger) const { - std::unique_ptr layer = CreateNNLayer(node); + std::unique_ptr layer = CreateNNLayer(model_builder, node); NodeAttrHelper helper(node); std::vector perm = helper.Get("perm", std::vector());