ZhennanQin commented on a change in pull request #16131: Fix for duplicate
subgraph inputs/outputs
URL: https://github.com/apache/incubator-mxnet/pull/16131#discussion_r324939057
##########
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:
@samskalicky Before this PR, backend still has the chance to remove the
duplicated sym.outputs in CreateSubgraphNode(). The existing APIs are enough to
solve the problem you're trying to fix, so I think this PR is to make all of
this happen in a automatic way. But the change in connectSubgraphOutputs()
doesn't achieve that goal, but making the solution semi-automatic.
From a backend developer position, I rather prefer to have full control of
my code(current APIs works in this way), or making most of the code
automatically(the way I suggest to change). For the semi-automatic solution,
developer have to spend extra effort to figure out which parts are handled
automatically(sym.outputs), and which parts are not(connectSubgraphOutputs).
And also increased the change that backend developer to make a bug when
overriding connectSubgraphOutputs(I can't find any reason for connecting
entries with same value to different sym.outputs, but by mistake).
Anyway, I don't against to make the change in your way. At least maybe we
should add some comments in connectSubgraphOutputs to highlight the difference
between sym.outputs and output_entries, and let developer notice that they need
to handle the duplicated entires carefully?
----------------------------------------------------------------
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
