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

Shrestha/add control dependencies in ngraph cluster #413

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

shresthamalik
Copy link
Contributor

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.

src/ngraph_builder.cc Outdated Show resolved Hide resolved
src/ngraph_builder.cc Outdated Show resolved Hide resolved
src/ngraph_builder.cc Outdated Show resolved Hide resolved
src/ngraph_builder.cc Show resolved Hide resolved
src/ngraph_builder.cc Outdated Show resolved Hide resolved
src/ngraph_builder.cc Outdated Show resolved Hide resolved

if (!is_nhwc) {
SaveNgOp(first_ng_op_map, op->name(), ng_input);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

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

Copy link
Contributor

@sayantan-nervana sayantan-nervana left a 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) )

src/ngraph_builder.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@mingshan-wang mingshan-wang left a 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.

Copy link
Contributor

@sindhu-nervana sindhu-nervana left a 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 <20289882+shresthamalik@users.noreply.github.com>
@sayantan-nervana
Copy link
Contributor

sayantan-nervana commented Feb 5, 2019

In our discussion @shresthamalik pointed out this graph:

    W
    |
    v
X-->Y-->*I--->Z
|        ^
|________|

Data edges: X->Y, I->Z, X->I, W->Y. Control edges (starred): Y->*I. I is identity node

If we attach the control edge Y->I to Y->X (parent of I), then we introduce a cycle in nodes X,Y

In such cases we cannot attach control edges to parents, and . we cannot honor the Y->*I control edge (unless we attach the control edge to children Z)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants