Skip to content

Conversation

@skottmckay
Copy link
Contributor

Simplify logic around creating relationship between nodes for implicit NodeArg usage. Allows using an initializer from multiple levels up to not fail. We would need to accumulate a list of initializers from all levels up otherwise, and doing so doesn't add any value vs inverting the logic and simply creating relationships based on known outputs.

Improve a comment to clarify when the parent graph NodeArg lookup kicks in.

…t NodeArg usage. Allows using an initializer from multiple levels up to not fail. We would need to accumulate a list of initializers from all levels up otherwise, and doing so doesn't add any value.

Improve a comment to clarify when the parent graph NodeArg lookup kicks in.
@skottmckay skottmckay requested a review from a team December 18, 2018 00:15
@linkerzhang
Copy link
Contributor

linkerzhang commented Dec 18, 2018

This is a high-level comment please.

I'm seeing that we're having only one global namespace in a model now as sub-graphs now can refer to an initializer several levels-up. is this expected?

@skottmckay
Copy link
Contributor Author

This is a high-level comment please.

I'm seeing that we're having only one global namespace in a model now as sub-graphs now can refer to an initializer several levels-up. is this expected?

I'm not aware of anything in ONNX that provides implicit namespacing. Is there something?

I guess it would be possible to use Graph.name as a prefix, and accumulate those prefixes as you descend into nested subgraphs to provide implicit namespacing if that was desirable.

That seems orthogonal to this change which is about an initializer coming from multiple levels up, regardless of whether there was some additional namespace information added. The initializer is in scope either way.

@gramalingam
Copy link
Contributor

I don't fully understand Ke's question: from the ONNX specification perspective,
(a) There is no difference between the immediately containing outer scope and an outer scope several levels up. The behavior should be same in all cases.
(b) There is no difference between an initializer X and a tensor Y computed as an output by some op, in terms of a subgraph referencing X or Y. Both are allowed.
(c) The question that is being discussed in ONNX (onnx/onnx#1665 ) is about whether a name X defined in an outer scope can be redefined to mean something else in a nested inner scope (and related issues).
(d) There is no notion of qualified names in ONNX at this point (as far as I know).

@skottmckay skottmckay merged commit beb326f into master Dec 18, 2018
@skottmckay skottmckay deleted the scmckay/HandleUsingInitializerFromMultipleLevelsAboveSubgraph branch December 18, 2018 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants