From 9fb8867a2439ca28814540f24af9e0e2431cc34d Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Fri, 2 Aug 2019 07:23:41 +1000 Subject: [PATCH] Don't create implicit input for outer scope value if there is a subgraph input with the same name. (#1186) * If there is an outer scope value that matches a subgraph input, don't create an implicit input from the outer scope value. Minor unrelated change for issue noticed while debugging: Use unordered_set for implicit inputs so we don't add them multiple times. * Add unit test based on onnx issue. --- include/onnxruntime/core/graph/graph.h | 2 +- onnxruntime/core/graph/graph.cc | 50 +++++++------ onnxruntime/test/framework/test_utils.h | 19 ++++- .../providers/cpu/controlflow/loop_test.cc | 57 +++++++++++++++ ...graph_input_shadows_outer_scope_value.onnx | 73 +++++++++++++++++++ 5 files changed, 177 insertions(+), 24 deletions(-) create mode 100644 onnxruntime/test/testdata/subgraph_input_shadows_outer_scope_value.onnx diff --git a/include/onnxruntime/core/graph/graph.h b/include/onnxruntime/core/graph/graph.h index f5c5209b5cc2a..b626a7541713f 100644 --- a/include/onnxruntime/core/graph/graph.h +++ b/include/onnxruntime/core/graph/graph.h @@ -853,7 +853,7 @@ class Graph { // Build and verify node connection (edges). // Verify NodeArg name/type/shape matching correctly. - common::Status BuildConnections(std::vector& outer_scope_node_args_consumed); + common::Status BuildConnections(std::unordered_set& outer_scope_node_args_consumed); common::Status VerifyNoDuplicateName(); diff --git a/onnxruntime/core/graph/graph.cc b/onnxruntime/core/graph/graph.cc index 57ae2855aa840..e26f91117c509 100644 --- a/onnxruntime/core/graph/graph.cc +++ b/onnxruntime/core/graph/graph.cc @@ -898,7 +898,7 @@ void Graph::RemoveEdge(NodeIndex src_node_index, NodeIndex dst_node_index, int s } GSL_SUPPRESS(es .84) // ignoring return value from unordered_map::insert causes noisy complaint -Status Graph::BuildConnections(std::vector& outer_scope_node_args_consumed) { +Status Graph::BuildConnections(std::unordered_set& outer_scope_node_args_consumed) { const std::unordered_set& outer_scope_node_args = resolve_context_.outer_scope_node_args; std::unordered_set inner_nodes; @@ -908,7 +908,7 @@ Status Graph::BuildConnections(std::vector& outer_scope_node_args_c for (auto* node : resolve_context_.nodes_with_subgraphs) { for (auto& subgraph : node->MutableSubgraphs()) { - std::vector node_args_consumed; + std::unordered_set node_args_consumed; ORT_RETURN_IF_ERROR(subgraph->BuildConnections(node_args_consumed)); for (auto& node_arg_name : node_args_consumed) { @@ -918,7 +918,7 @@ Status Graph::BuildConnections(std::vector& outer_scope_node_args_c // it's a node arg from outside this graph's scope, so add that to the list we return // so that we can add the dependency at the next level up. this happens if you have multiple // levels of subgraphs between the graph with the original NodeArg and the subgraph with implicit usage. - outer_scope_node_args_consumed.push_back(node_arg_name); + ORT_IGNORE_RETURN_VALUE(outer_scope_node_args_consumed.insert(node_arg_name)); if (!parent_graph_) { return ORT_MAKE_STATUS( @@ -987,25 +987,31 @@ Status Graph::BuildConnections(std::vector& outer_scope_node_args_c continue; } - auto output_arg_iter = resolve_context_.output_args.find(input_arg->Name()); - if (resolve_context_.output_args.end() == output_arg_iter) { - // No such output_arg matching this input_arg. - // This input arg should be fed when running evaluation. - // See if it's present in the outer scope. If so it will be 'fed' by the execution frame - // providing access to the OrtValue from the outer scope. Pass the name back up so nodes can - // be linked correctly at that level. - if (outer_scope_node_args.find(input_arg->Name()) != outer_scope_node_args.cend()) { - outer_scope_node_args_consumed.push_back(input_arg->Name()); - } + const auto& input_arg_name = input_arg->Name(); + auto output_arg_iter = resolve_context_.output_args.find(input_arg_name); + if (resolve_context_.output_args.end() != output_arg_iter) { + // The input to this node is an output from a previous node in this graph. + // Create relationship between this node (node), and the node providing the output (output_node). + Node& output_node = *output_arg_iter->second.first; + AddEdge(output_node.Index(), node.Index(), output_arg_iter->second.second, input_slot_index); - continue; + inner_nodes.insert(&output_node); + } else { + // the value is either an input, an initializer, or coming from outer scope. we only need to take action + // if coming from outer scope, so first check if this is a subgraph (otherwise there is no outer scope). + if (parent_graph_ != nullptr) { + // make sure it's not an input or initializer first as those override any outer scope values + if (resolve_context_.inputs_and_initializers.find(input_arg_name) == + resolve_context_.inputs_and_initializers.cend()) { + // If it is present in the outer scope it will be 'fed' by the execution frame + // providing access to the OrtValue from the outer scope. Pass the name back up so nodes can + // be linked correctly at that level. + if (outer_scope_node_args.find(input_arg_name) != outer_scope_node_args.cend()) { + ORT_IGNORE_RETURN_VALUE(outer_scope_node_args_consumed.insert(input_arg_name)); + } + } + } } - - // Create relationship between this node (node), and the node providing the output (output_node). - Node& output_node = *output_arg_iter->second.first; - AddEdge(output_node.Index(), node.Index(), output_arg_iter->second.second, input_slot_index); - - inner_nodes.insert(&output_node); } } else if (node.OutputDefs().empty()) { // This is a useless node. @@ -1015,7 +1021,7 @@ Status Graph::BuildConnections(std::vector& outer_scope_node_args_c } return Status::OK(); -} +} // namespace onnxruntime void Graph::ReverseDFSFrom(const std::vector& from, const std::function& enter, @@ -1869,7 +1875,7 @@ Status Graph::Resolve(bool no_proto_sync_required) { // recursively set the outer scope node args. ORT_RETURN_IF_ERROR(SetOuterScopeNodeArgs(resolve_context_.outer_scope_node_args)); - std::vector outer_scope_node_args_consumed; + std::unordered_set outer_scope_node_args_consumed; // recursively build connections between nodes in this graph and all subgraphs ORT_RETURN_IF_ERROR(BuildConnections(outer_scope_node_args_consumed)); diff --git a/onnxruntime/test/framework/test_utils.h b/onnxruntime/test/framework/test_utils.h index 27b1d06596d37..9b36d8bd72902 100644 --- a/onnxruntime/test/framework/test_utils.h +++ b/onnxruntime/test/framework/test_utils.h @@ -10,6 +10,8 @@ #include "core/providers/cpu/cpu_execution_provider.h" #include "core/framework/ml_value.h" +#include "gsl/gsl_algorithm" + #ifdef USE_CUDA #include "core/providers/cuda/cuda_execution_provider.h" #endif @@ -48,6 +50,20 @@ IExecutionProvider* TestOpenVINOExecutionProvider(); IExecutionProvider* TestNnapiExecutionProvider(); #endif +template +inline void CopyVectorToTensor(const std::vector& value, Tensor& tensor) { + gsl::copy(gsl::make_span(value), tensor.MutableDataAsSpan()); +} + +// vector is specialized so we need to handle it separately +template <> +inline void CopyVectorToTensor(const std::vector& value, Tensor& tensor) { + auto output_span = tensor.MutableDataAsSpan(); + for (size_t i = 0, end = value.size(); i < end; ++i) { + output_span[i] = value[i]; + } +} + template void CreateMLValue(AllocatorPtr alloc, const std::vector& dims, const std::vector& value, OrtValue* p_mlvalue) { @@ -57,8 +73,9 @@ void CreateMLValue(AllocatorPtr alloc, const std::vector& dims, const s shape, alloc); if (value.size() > 0) { - memcpy(p_tensor->MutableData(), &value[0], element_type->Size() * shape.Size()); + CopyVectorToTensor(value, *p_tensor); } + p_mlvalue->Init(p_tensor.release(), DataTypeImpl::GetType(), DataTypeImpl::GetType()->GetDeleteFunc()); diff --git a/onnxruntime/test/providers/cpu/controlflow/loop_test.cc b/onnxruntime/test/providers/cpu/controlflow/loop_test.cc index d4293450f17ab..36a62bf4b90fe 100644 --- a/onnxruntime/test/providers/cpu/controlflow/loop_test.cc +++ b/onnxruntime/test/providers/cpu/controlflow/loop_test.cc @@ -11,6 +11,7 @@ #include "test/providers/provider_test_utils.h" #include "test/util/include/default_providers.h" +#include "test/framework/test_utils.h" using namespace ONNX_NAMESPACE; @@ -573,6 +574,62 @@ TEST(Loop, InfiniteLoopTermination) { terminator_thread.join(); } +// Regression test that a subgraph input overrides an outer scope value of the same name. +// Replicate issue from https://github.com/onnx/onnx/issues/2082 +TEST(Loop, SubgraphInputShadowsOuterScopeValue) { + SessionOptions so; + so.session_logid = "SubgraphInputShadowsOuterScopeValue"; + + InferenceSession session_object{so, &DefaultLoggingManager()}; + Status st; + ASSERT_TRUE((st = session_object.Load("testdata/subgraph_input_shadows_outer_scope_value.onnx")).IsOK()) << st; + ASSERT_TRUE((st = session_object.Initialize()).IsOK()) << st; + + // prepare inputs + std::vector scalar = {1}; + std::vector a = {3.f}, b = {6.f}; + std::vector trip_count = {10}; + std::vector keep_going = {true}; + + NameMLValMap feeds; + OrtValue ml_value; + + CreateMLValue(TestCPUExecutionProvider()->GetAllocator(0, OrtMemTypeDefault), scalar, a, &ml_value); + feeds.insert(std::make_pair("a", ml_value)); + CreateMLValue(TestCPUExecutionProvider()->GetAllocator(0, OrtMemTypeDefault), scalar, b, &ml_value); + feeds.insert(std::make_pair("b", ml_value)); + CreateMLValue(TestCPUExecutionProvider()->GetAllocator(0, OrtMemTypeDefault), scalar, trip_count, &ml_value); + feeds.insert(std::make_pair("max_trip_count", ml_value)); + CreateMLValue(TestCPUExecutionProvider()->GetAllocator(0, OrtMemTypeDefault), scalar, keep_going, &ml_value); + feeds.insert(std::make_pair("keep_going_inp", ml_value)); + + // prepare outputs + std::vector output_names{"b", "user_defined_vals"}; + std::vector fetches; + + // Now run + onnxruntime::RunOptions run_options; + st = session_object.Run(run_options, feeds, output_names, &fetches); + ASSERT_TRUE(st.IsOK()) << st; + ASSERT_EQ(2, fetches.size()); + + // prepare expected outputs + float expected_value_b = 6.f; + std::vector expected_dims_user_defined_vals = {2, 1}; + std::vector expected_user_defined_vals = {-6.f, 12.f}; + + auto& b_out = fetches[0].Get(); + TensorShape expected_shape(scalar); + ASSERT_EQ(expected_shape, b_out.Shape()); + ASSERT_EQ(b_out.DataAsSpan()[0], expected_value_b); + + auto user_defined_vals_out = fetches[1].Get().DataAsSpan(); + ASSERT_EQ(expected_user_defined_vals.size(), static_cast(user_defined_vals_out.size())); + for (size_t i = 0, end = expected_user_defined_vals.size(); i < end; ++i) { + ASSERT_THAT(user_defined_vals_out[i], testing::FloatEq(expected_user_defined_vals[i])); + } +} + #ifdef USE_CUDA // test that when part of the subgraph run on CUDA it executes successfully TEST(Loop, MixedExecutionProviders) { diff --git a/onnxruntime/test/testdata/subgraph_input_shadows_outer_scope_value.onnx b/onnxruntime/test/testdata/subgraph_input_shadows_outer_scope_value.onnx new file mode 100644 index 0000000000000..1821b393d0481 --- /dev/null +++ b/onnxruntime/test/testdata/subgraph_input_shadows_outer_scope_value.onnx @@ -0,0 +1,73 @@ +:µ +— +max_trip_count +keep_going_inp +bb_loopmy_localuser_defined_vals"Loop*Æ +body2º + +a +bmy_local"Add + +a +bb_loop"Sub +' +my_local +b_loop +keep_going"Greater +( +b_loop +b_loopuser_defined_vals"AddbodyZ + iteration_num + + +Z +keep_going_inp + +  +Z +b + + +b + +keep_going + +  +b +b_loop + + +b +my_local + + +b +user_defined_vals + + + outerZ +a + + +Z +b + + +Z +keep_going_inp + +  +Z +max_trip_count + + +b +b + + +b# +user_defined_vals +  + + +B