Clarify namescopes in the presence of nested subgraphs

[gramalingam]

We need to clarify the treatment of names occurring in nested subgraphs, now that we have control-flow ops that use nested subgraphs.

The agreed part is that a nested subgraph can contain references to a name X defined in an outer scope (graph).

The following are less obvious (pros/cons with the different choices):

(1) Should reuse of same name X for different outputs in different graphs be permitted in (a) some cases, (b) all cases, or (c) no cases?

(2) What about node names? Are they required to be unique across a graph and a nested subgraph?

Here is my understanding of what the checker currently does:

X = ADD(…) //
{ // nested subgraph 1
X = MULTIPLY(…) // forbidden by checker
Y = ADD(…) // permitted by checker
Z = ADD(…) // permitted by checker
}
Y = MULTIPLY(…) // permitted by checker
{ // nested subgraph 2
Z = MULTIPLY(…) // permitted by checker
}

Note that forbidding reuse in all above cases is easier to explain/document!

Please indicate if there is any specific requirement or usecase for a particular choice above.

[houseroad]

The changes look good to me.

[linkerzhang]

Confirm: does this mean all node input/output names across main graph and subgraphs have to be unique? if this is the case, then what's the "namespace" here mean please? With this clarification, I assume there's only ONE namespace in a model now. Every name has to be unique in this ONE namespace. Right?

[gramalingam]

We have namespaces as described in the table just below (node names are in a different namespace from tensor/value names, for example). We can think of different graphs as a different dimension: let us call them namescopes. So, tensor names must be unique across a graph and a subgraph (one namescope). But a tensor name can be the same as a node name (different namespaces).

[zrphercule]

LGTM

[gramalingam]

Hi, I updated the documentation to be consistent with the checker. However, there are some tricky issues (non-uniform handling of name reuse/clash), so I would like to bring them up. Please clarify if the existing implementation is intentional or whether it needs to be fixed.

A name X defined in an outer scope cannot be reused for a different tensor in a subgraph. However, this is permitted for the subgraph inputs. Is this intentional? Why this non-uniform treatment?

[gramalingam]

I want to follow up on the earlier discussion. Consider what the current checker does with the following example of reuse of names defined in an outer graph in a subgraph (for a different value):

graph {
X1 = ..
X2 = ..
… control-flow-op
sub-graph (X1) { // permitted by checker
X2 = .. // not permitted by checker
X3 = .. // permitted by checker
}
X3 = ..
}

This looks a bit odd. Why do we not want to permit the redefinition of X2 above? One possible explanation is that we want to avoid the following scenario:

graph () {
Y = .. // outer scope Y
.. sub-graph {
Z = Y; // reference to outer scope Y
Y = ..; // redefinition of Y
}
}

It seems a reasonable goal to avoid a case where a name Y in a particular scope S is used to both refer to an outer scope variable (that is, defined outside scope S) as well as a variable define in scope S. (A similar situation is when the reference to the outer scope Y occurs in a nested sub-scope within S.)

Suggested solution: we forbid redefinitions of names defined in an outer scope only in the above situation. In particular, we allow the redefinition of X2 in the original example above.

Does this seem reasonable? This would also explain why we allow the redefinition of X1 in the first example.

Note that forbidding the redefinition of X2 has the following consequence: consider a graph G that is completely self-contained (that is, it has no references to an undefined name). Ideally, we should be allowed to use it as a sub-graph anywhere without worrying if it uses a name that conflicts with an outer-scope name. The existing scheme doesn't give us this desirable compositionality property. Fixing it as suggested above will give us this property as well.

Please note that this fix will make the checker more complex. I can look into how to update the checker if this proposal looks reasonable.

[houseroad]

LGTM.