Don't create implicit input for outer scope value if there is a subgraph input with the same name. #1186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… create an implicit input from the outer scope value. Minor unrelated change for issue noticed while debugging: Use unordered_set for implicit inputs so we don't add them multiple times.
gramalingam
reviewed
Aug 1, 2019
| if (outer_scope_node_args.find(input_arg_name) != outer_scope_node_args.cend()) { | ||
| // if it matches an outer scope value make sure it's not also a graph input or initializer | ||
| // as the ONNX checker currently allows a graph input to shadow an outer scope value | ||
| if (resolve_context_.inputs_and_initializers.find(input_arg_name) == |
There was a problem hiding this comment.
Minor nit: Conceptually, this belongs together with the search in line 991 (search the locally-defined names, which is the union of output_args and inputs and initializers). While it has the same effect, it might be a bit easier to understand if we move this check up there (basically swapping the two checks in 999 and 1002 around).
gramalingam
previously approved these changes
Aug 1, 2019
…aphInputNotShadowingOuterScopeValue
gramalingam
approved these changes
Aug 1, 2019
skottmckay
deleted the
skottmckay/FixSubgraphInputNotShadowingOuterScopeValue
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If there is an outer scope value that matches a subgraph input, don't create an implicit input from the outer scope value. Doing so leads to both being in scope when executing the subgraph which is incorrect. onnx/onnx#2082
Minor unrelated change for issue noticed while debugging: Use unordered_set for implicit inputs so we don't add them multiple times.