samskalicky commented on a change in pull request #16131: Fix for duplicate
subgraph inputs/outputs
URL: https://github.com/apache/incubator-mxnet/pull/16131#discussion_r324915391
##########
File path: src/operator/subgraph/subgraph_property.h
##########
@@ -296,8 +296,20 @@ class SubgraphProperty {
*/
virtual void ConnectSubgraphOutputs(const nnvm::NodePtr subgraph_node,
std::vector<nnvm::NodeEntry*>*
output_entries) const {
+ // Collapse output_entries pointing to same NodeEntry
Review comment:
Hey Guys,
I think we're getting offtrack here. Lets just clarify what we're working
toward. We want the subgraph to only have unique outputs, so this code is
necessary in creating the sym.outputs correctly:
https://github.com/apache/incubator-mxnet/pull/16131/files#diff-962e4e83b570943c087e477058485e25R578-R587
Then, the problem is we need to wire up the those unique outputs to each
entry in output_entries. Since we pass the new symbol "sym" into
connectSubgraphOutputs indirectly via the node "n", technically the set of
unique outputs from the subgraph is already available in the
connectSubgraphOutputs function.
To clarify, the ideal solution would be to pass in a map from output node to
output entry and then let connectSubgraphOutputs just wire them up as it sees
fit.
Do we agree that we do not want to change the API of connectSubgraphOutputs
to enable this? Is there any reason not to just do this now? it shouldnt be too
much work to go change, there is only a few subgraph properties that would need
to change:
https://github.com/apache/incubator-mxnet/search?q=connectSubgraphOutputs&unscoped_q=connectSubgraphOutputs
And I think the creators of those wouldnt mind just fixing this once and for
all now. This would go in the 1.6 release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
