-
Notifications
You must be signed in to change notification settings - Fork 16
Shrestha/add control dependencies in ngraph cluster #413
base: master
Are you sure you want to change the base?
Shrestha/add control dependencies in ngraph cluster #413
Conversation
|
||
if (!is_nhwc) { | ||
SaveNgOp(first_ng_op_map, op->name(), ng_input); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ng_filter and ng_input are different data flows, so this check I think is still valid
…ttps://github.com/NervanaSystems/ngraph-tf into shrestha/Add_control_dependencies_in_ngraph_cluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider this graph:
X---->Identity----->Z1
^ \
| \
Y Z2
Where X
produces the actual data and Y
has a control edge on the identity node. What this is saying is that "Do not start computing Z1 and Z2 till Y is done computing".
I think since we have no real op in place of identity, this constraint is probably not translatable right now?
Why might we see this structure? Let us say we have Z1 to Z100 that we want to hold off on computing till Y is done. Instead of attaching control edges from Y to Zi individually, we can attach a control edge to an identity node that feeds all the Zi nodes.
Note however that if we attach the control edge to X, we will conservatively maintain the constraint. Moving the control edge to your data parent is always safe.
Another possibility is moving the control edge to all the Zi nodes
In conclusion: If a TF node has no ng node to which we can attach incoming control edges, then we can either attach the control edge to the input ngraph node (which respects the intended constraints, but introduces additional constraints), or we can attach the control edge to the next nodes (this is the correct translation of the constraint, however it might need more bridge work, since we have to keep track of unattached control edges, which must be taken care of by the children/grandchildren nodes. (consider the scenario of X->identity->identity->Z, with a control edge incoming into the first identity. This graph structure is unlikely, but valid) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for making all the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for answering all questions.
Co-Authored-By: shresthamalik <[email protected]>
In our discussion @shresthamalik pointed out this graph:
Data edges: If we attach the control edge In such cases we cannot attach control edges to parents, and . we cannot honor the |
Add control dependency to the translated nGraph nodes. first_ng_op_map stores the entry point nGraph nodes and ng_op_map stores the exit point nGraph nodes for each Tf translated op. After the TF graph has been translated to nGraph, we add the control dependencies to the nGraph nodes using the stored entry and exit points.