From 7e6d052275b0d0ad377cda95cf0d7e6e819816be Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Sat, 28 May 2022 08:04:27 +1000 Subject: [PATCH] Add better error message for subgraph output coming directly from outer scope value. (#11638) * Add better message for subgraph output coming directly from outer scope value. * Use regex to match value name as the test model is processed in a different order on different platforms. --- onnxruntime/core/graph/graph.cc | 47 ++++++++++-------- onnxruntime/test/ir/graph_test.cc | 32 +++++++----- .../test/testdata/ort_github_issue_11536.onnx | Bin 0 -> 1508 bytes 3 files changed, 47 insertions(+), 32 deletions(-) create mode 100644 onnxruntime/test/testdata/ort_github_issue_11536.onnx diff --git a/onnxruntime/core/graph/graph.cc b/onnxruntime/core/graph/graph.cc index 61ed6ca0cf..5792702b41 100644 --- a/onnxruntime/core/graph/graph.cc +++ b/onnxruntime/core/graph/graph.cc @@ -598,7 +598,7 @@ Status Node::GetInstantiateFunctionBody(std::unique_ptr& output) const Status Node::InstantiateFunctionBody() { if (nullptr != func_body_) { - //already instantiated. + // already instantiated. return Status::OK(); } @@ -1277,16 +1277,16 @@ Graph::Graph(Graph& parent_graph, const Node& parent_node, ONNX_NAMESPACE::Graph parent_graph.strict_shape_type_inference_) { } -Graph::Graph(const Model& owning_model, - IOnnxRuntimeOpSchemaCollectionPtr schema_registry, - ONNX_NAMESPACE::GraphProto& subgraph_proto, - const std::unordered_map& domain_version_map, - const logging::Logger& logger, - bool strict_shape_type_inference) +Graph::Graph(const Model& owning_model, + IOnnxRuntimeOpSchemaCollectionPtr schema_registry, + ONNX_NAMESPACE::GraphProto& subgraph_proto, + const std::unordered_map& domain_version_map, + const logging::Logger& logger, + bool strict_shape_type_inference) : Graph(owning_model, &subgraph_proto, - domain_version_map, - owning_model.IrVersion(), + domain_version_map, + owning_model.IrVersion(), schema_registry, nullptr, nullptr, @@ -1347,10 +1347,17 @@ void Graph::InitializeStateFromModelFileGraphProto() { // Graph output is not found as any initializer. auto iter3 = graph_inputs.find(graph_output_name); if (graph_inputs.end() == iter3) { - // Graph output is not found as any graph input. - ORT_THROW( - "This is an invalid model. Graph output (", graph_output_name, - ") does not exist in the graph."); + if (parent_graph_ == nullptr || + parent_graph_->GetNodeArgIncludingParentGraphs(graph_output_name) == nullptr) { + // Graph output is not found as any graph input. + ORT_THROW("This is an invalid model. Graph output (", graph_output_name, ") does not exist in the graph."); + } else { + // Special case of a subgraph directly returning an outer scope value. This is not explicitly allowed + // by the ONNX spec, and supporting it would potentially be complicated. + ORT_THROW("This is an invalid model. Subgraph output (", graph_output_name, + ") is an outer scope value being returned directly. Please update the model to add an " + "Identity node between the outer scope value and the subgraph output."); + } } graph_outputs_.push_back(iter3->second); continue; @@ -2553,8 +2560,8 @@ Status Graph::VerifyNodeAndOpMatch(const ResolveOptions& options) { if (node.since_version_ == -1) { node.since_version_ = node.op_->since_version(); } - } - + } + ORT_RETURN_IF_ERROR(node.UpdateInputArgCount()); // currently an Op is required by ValidateVersion, so we use gsl::not_null to validate that. @@ -2913,7 +2920,7 @@ Status Graph::InjectExternalInitializedTensors(const InlinedHashMapHasFunction() && !schema->HasContextDependentFunction());*/ continue; } - + const auto& domain_version_map = func_ptr->Body().DomainToVersionMap(); // validate schema for each node in the function body can be found for (auto& n : func_ptr->Body().Nodes()) { @@ -505,7 +505,7 @@ TEST_F(GraphTest, LocalCustomRegistryWrongOpsetImportVersion) { ASSERT_TRUE(registry->RegisterOpSet(schema, "FakeTestDomain", 0, 1).IsOK()); ModelProto m; m.set_ir_version(3); - //Should be 1, but we put 11 herer so the model loading will fail + // Should be 1, but we put 11 herer so the model loading will fail ImportOpset(m, "FakeTestDomain", 11); GraphProto& g = *m.mutable_graph(); NodeProto* node = g.add_node(); @@ -540,7 +540,7 @@ TEST_F(GraphTest, ReverseDFS) { * node_4 (Add) ------------------- <-- request stop * | * SinkNode - */ + */ std::vector inputs; std::vector outputs; @@ -771,7 +771,7 @@ TEST_F(GraphTest, GraphConstruction_CheckIsAcyclic) { EXPECT_TRUE(equal_proto_1_and_2); // Load the model again to ensure that it's still the right thing. - //EXPECT_EQ(Model::Load(model_proto2, &model2), Status::OK()); + // EXPECT_EQ(Model::Load(model_proto2, &model2), Status::OK()); model2.reset(new Model(model_proto2, nullptr, *logger_)); Graph& graph2 = model2->MainGraph(); for (auto& node : graph2.Nodes()) { @@ -1238,7 +1238,7 @@ TEST_F(GraphTest, GraphConstruction_CheckGraphInputOutputOrderMaintained) { // serialize and reload so we check the loaded from proto path in SetGraphInputsOutputs auto proto = model.ToProto(); std::string s1; - //std::stringstream s1; + // std::stringstream s1; model.ToProto().SerializeToString(&s1); ModelProto model_proto; @@ -1307,7 +1307,7 @@ TEST_F(GraphTest, UnusedInitializerAndNodeArgsAreIgnored) { // serialize and reload so we check the loaded from proto path in SetGraphInputsOutputs auto proto = model.ToProto(); std::string s1; - //std::stringstream s1; + // std::stringstream s1; model.ToProto().SerializeToString(&s1); ModelProto model_proto; @@ -1426,7 +1426,7 @@ TEST_F(GraphTest, GraphConstruction_TypeInference) { * node_4 (Max) * | * SinkNode - */ + */ std::vector inputs; std::vector outputs; @@ -1780,8 +1780,7 @@ TEST_F(GraphTest, InjectExternalInitializedTensors) { Tensor::InitOrtValue(DataTypeImpl::GetType(), data_shape, tensor_data.data(), OrtMemoryInfo(onnxruntime::CPU, OrtAllocatorType::OrtDeviceAllocator), ort_value); const InlinedHashMap injection_initializers = { - {initializer_name, ort_value} - }; + {initializer_name, ort_value}}; // We do not need actual files there since we are not going to load it. const auto tensor_data_dir_path = Path::Parse(ToPathString(".")); @@ -1946,7 +1945,7 @@ TEST_F(GraphTest, SparseInitializerHandling) { ValidateSparseTensorProto(model_proto_get.graph().sparse_initializer().at(0)); } } -#endif //!defined(DISABLE_SPARSE_TENSORS) +#endif //! defined(DISABLE_SPARSE_TENSORS) TEST_F(GraphTest, SetInputsAndSetOutputs_NewInputAndOutput) { std::shared_ptr model; @@ -2107,5 +2106,14 @@ TEST_F(GraphTest, ConstantsBecomeInitializersAndInputs) { Status st = Model::Load(std::move(m), model, nullptr, *logger_); ASSERT_TRUE(st.IsOK()) << st.ErrorMessage(); } + +TEST_F(GraphTest, SubgraphOutputIsOuterScopeValue) { + std::shared_ptr model; + common::Status st = Model::Load(ORT_TSTR("./testdata/ort_github_issue_11536.onnx"), model, nullptr, *logger_); + ASSERT_FALSE(st.IsOK()); + EXPECT_THAT(st.ErrorMessage(), + ::testing::ContainsRegex("Subgraph output \\(.*\\) is an outer scope value being returned directly.")); +} + } // namespace test } // namespace onnxruntime diff --git a/onnxruntime/test/testdata/ort_github_issue_11536.onnx b/onnxruntime/test/testdata/ort_github_issue_11536.onnx new file mode 100644 index 0000000000000000000000000000000000000000..696d055edf201493592a28bab376543d0732c949 GIT binary patch literal 1508 zcmb7^&2HL25XWt>!HkI-EtfQ{D%H8Uii%AL6(^UQN587s<|mHu-v^g23aaH7q%F*yyaUy?h)%;13HSi()yw6|tG`cE9(k)Fg;S490wZI1$oN_h zLXVR)1ZLFTgZ={LLe%h4)G&AwDwiu%)m;_!Ttmn`w>g#AQG^XOEV_65U*YW?@_M@7 z?tCcV@L%8%fqRbw51s}*(7>v_Bfx5JC0S1c`#WIVevGO!Z1Hm#><{me!Cf7%!^bGb zdR3{zhf%o%@_-X5>U(fQ({(~W=b1Qf{ECPupOh$ZeLUL3=-?na!FBr}J4fSM8mglK zX)*c`;rq~-WGsnG-vvv+pBY=T#v8ccPYCQ~#L21Dr(W|0|G4NK?A!!OemRR(R?yUI zebN7Z2dov%mm_0TAJs<`FllTiQw literal 0 HcmV?d00001