Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix for duplicate subgraph inputs/outputs #16131

Merged
merged 34 commits into from
Sep 10, 2020
+81 −18
Merged

Fix for duplicate subgraph inputs/outputs #16131

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
b010ffa
fix for duplicate inputs
Sep 9, 2019
a6ad15a
fixed error
Sep 9, 2019
00499c9
fixed whitespace
Sep 9, 2019
87b665e
Remove duplicate outputs from subgraphs
Sep 10, 2019
8045bab
changed subgraph to create map of outputs
Sep 17, 2019
82df19d
added static_cast
Sep 17, 2019
77f3f31
changed map<int,v> to vector
Sep 17, 2019
a4b42db
sanity fix
Sep 17, 2019
c6077dd
sanity2
Sep 17, 2019
3631bc7
updated backends with new connectSubgraphOutputs API
Sep 17, 2019
fdd477b
fixed map creation logic
Sep 17, 2019
7a210d6
added updates for reattach function
samskalicky Jan 16, 2020
a610f1c
creating node only if it is not an input to subgraph
Feb 13, 2020
4db8eec
creating object based on var_name only
Feb 13, 2020
be9c885
updating ConnectSubgraphOutputs for mkldnn_elemwisemul_post_quantize_…
Feb 14, 2020
41c5e4c
add debug prints to debug error in CI
samskalicky Feb 15, 2020
fde8a05
remove prints
samskalicky Feb 15, 2020
acc1e73
added prints to debug in the CI
samskalicky Feb 15, 2020
10652c8
Merge branch 'subgraph_fix' of https://github.com/samskalicky/incubat…
samskalicky Feb 15, 2020
b49b431
Merge branch 'master' into subgraph_fix
mseth10 Apr 20, 2020
cd4f4d7
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
Aug 30, 2020
6a20b0e
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
Aug 30, 2020
324f29b
revert changes
Aug 30, 2020
e0abc55
reverted changes
Aug 30, 2020
93a8f35
deduplicaated inputs to subgraph
Aug 30, 2020
d155801
deduplicated subgraph inputs
Aug 30, 2020
6deea77
simplified inputs
Aug 30, 2020
270a8fc
cleaned up
Aug 30, 2020
165bbd1
deduplicate outputs
Aug 31, 2020
5572d4c
cleand up
Aug 31, 2020
7672a5f
added deduplication to subgraph node outputs
Aug 31, 2020
e249d71
fixed prev compare
Aug 31, 2020
4a49810
fixed issue with inputs and added test
Sep 10, 2020
af9f177
fixd whitespace, removed prints
Sep 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions src/operator/subgraph/build_subgraph.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,10 +537,11 @@ void FindOutputEntries(nnvm::Graph* g,
*/
void CutGraphInputs(const std::vector<nnvm::NodeEntry*> &input_entries,
std::vector<nnvm::NodeEntry> *orig_entries,
std::vector<nnvm::NodeEntry> *unique_inputs,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an alternative implementation that does not need t track the actual unique inputs separately.
A counter would allow us to correctly modify orig_entries into unique_entries.
This keeps the function signature unchanged and minimizes changes elsewhere.

It is already working in a private build. I add it here for consideration if desired

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @HahTK, but we cant modify orig_entries since we need it unmodified for ReattachGraphInputs:
https://github.com/apache/incubator-mxnet/blob/e249d71ad4621afcba7f0f3af77095a3f9a4bc83/src/operator/subgraph/build_subgraph.cc#L578
this is used to reject a subgraph from the reviewSubgraph API:
https://github.com/apache/incubator-mxnet/blob/e249d71ad4621afcba7f0f3af77095a3f9a4bc83/src/operator/subgraph/build_subgraph.cc#L657
the example you provided doesnt work in the latest version of MXNet where you can reject a subgraph by returning nullptr from CreateSubgraphNode here: https://github.com/apache/incubator-mxnet/blob/e249d71ad4621afcba7f0f3af77095a3f9a4bc83/src/operator/subgraph/build_subgraph.cc#L630-L633

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed change was tested on 1.5.1 and it seems like newer versions of MXNet have new features that needs orig_entries to be preserved. So acknowledged !

const bool skip_var = false) {
orig_entries->resize(input_entries.size());
// map for creating unique var nodes for deduplicating entries from the same node
std::unordered_map<std::string, int> name_count_map;
std::unordered_map<std::string, nnvm::NodeEntry> name_count_map;
for (size_t i = 0; i < input_entries.size(); ++i) {
nnvm::NodeEntry *e = input_entries[i];
// If the node is a variable itself, we may want to skip the node.
Expand All @@ -556,14 +557,16 @@ void CutGraphInputs(const std::vector<nnvm::NodeEntry*> &input_entries,
const std::string& var_name = output_names[0];
auto it = name_count_map.find(var_name);
if (name_count_map.end() == it) {
name_count_map.emplace(var_name, 0);
// first use of this node as input to subgraph
unique_inputs->push_back(*e);
nnvm::ObjectPtr n = nnvm::CreateVariableNode(var_name + std::to_string(0));
*e = nnvm::NodeEntry{n, 0, 0};
// store node for re-use
name_count_map.emplace(var_name, *e);
} else {
++(it->second);
// other use of same node as input to subgraph
*e = it->second;
}
nnvm::ObjectPtr n = nnvm::CreateVariableNode(
var_name + std::to_string(name_count_map[var_name]));

*e = nnvm::NodeEntry{n, 0, 0};
}
}

Expand Down Expand Up @@ -596,7 +599,8 @@ void CreateSubgraphNode(nnvm::Graph* g,
std::vector<nnvm::NodeEntry*> input_entries;
FindInputEntries(*g, simple_nodes, subgraph_nodes, *entry_top_order_map, &input_entries);
std::vector<nnvm::NodeEntry> orig_input_entries;
CutGraphInputs(input_entries, &orig_input_entries, false);
std::vector<nnvm::NodeEntry> unique_inputs;
CutGraphInputs(input_entries, &orig_input_entries, &unique_inputs, false);
#if DEBUG_SUBGRAPH
PrintNodeEntries(input_entries);
LOG(INFO) << "Searching for output entries...";
Expand All @@ -611,14 +615,14 @@ void CreateSubgraphNode(nnvm::Graph* g,
sym.outputs[i] = *output_entries[i];
}
const SubgraphPropertyPtr& subg_prop = g->GetAttr<SubgraphPropertyPtr>("subgraph_property");
subg_prop->InitSubgraphInputs(&input_entries, &orig_input_entries);
subg_prop->InitSubgraphInputs(&input_entries, &unique_inputs);
nnvm::ObjectPtr n = subg_prop->CreateSubgraphNode(sym, subgraph_selector, subgraph_id);
// CreateSubgraphNode returns NULL if subgraph property determines that subgraph is sub-optimal
// In that case, subgraph node is not created and graph is not modified
if (n) {
// Connect the external nodes to the subgraph node.
subg_prop->ConnectSubgraphOutputs(n, &output_entries);
subg_prop->ConnectSubgraphInputs(n, &input_entries, &orig_input_entries);
subg_prop->ConnectSubgraphInputs(n, &input_entries, &unique_inputs);

const auto& indexed_graph = g->indexed_graph();
for (size_t i = 0; i < n->inputs.size(); ++i) {
Expand Down
21 changes: 20 additions & 1 deletion src/operator/subgraph/subgraph_property.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,27 @@ class SubgraphProperty {
*/
virtual void ConnectSubgraphOutputs(const nnvm::ObjectPtr subgraph_node,
std::vector<nnvm::NodeEntry*>* output_entries) const {
// Collapse output_entries pointing to same NodeEntry
samskalicky marked this conversation as resolved.
Show resolved Hide resolved
// Outputs are ordered, only neighboring nodes can point to same NodeEntry
std::unordered_map<std::string, nnvm::NodeEntry> name_count_map;

uint32_t idx = 0;
for (size_t i = 0; i < output_entries->size(); ++i) {
*output_entries->at(i) = nnvm::NodeEntry{subgraph_node, static_cast<uint32_t>(i), 0};
// get node name
nnvm::Symbol sym;
sym.outputs.push_back(*output_entries->at(i));
const auto output_names = sym.ListOutputNames();
CHECK_EQ(output_names.size(), 1U);
const std::string& var_name = output_names[0];

auto it = name_count_map.find(var_name);
// if it is not in the map yet, add it and increment the output index
if (name_count_map.end() == it) {
name_count_map.emplace(var_name, nnvm::NodeEntry{subgraph_node, idx, 0});
idx++;
}
// set output to index from map
*output_entries->at(i) = name_count_map[var_name];
samskalicky marked this conversation as resolved.
Show resolved Hide resolved
}
}
/*!
Expand Down