Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip existing const node in _parse_graph_input #2000

Merged
merged 2 commits into from
Jul 21, 2022
Merged

Skip existing const node in _parse_graph_input #2000

merged 2 commits into from
Jul 21, 2022

Conversation

q-ycong-p
Copy link
Contributor

If the Graph object being constructed already contains a node of
the name of a Const node in orginal graph, do not add as input.

Signed-off-by: congyc congyc@amazon.com

Skip existing const node in _parse_graph_input
aa5f3ac 
If the Graph object being constructed already contains a node of
the name of a Const node in orginal graph, do not add as input.

Signed-off-by: Yu Cong <congyc@amazon.com>
@q-ycong-p
Copy link
Contributor Author

q-ycong-p commented Jul 15, 2022 •
edited

There're two points I'd like to raise for conversion:

  1. Reason for the PR change: ONNX < IR-4 has initializers added as graph inputs. This leads to models such as non_quantized.onnx (attached within repro_issue.zip) to not be optimized as intended. See optimized_wrong.onnx (in repro_issue.zip) for the incorrect optimization prior to PR change, and optimized_correct.onnx (in repro_issue.zip) for the expected optimization post to PR change.

  2. Why change a unit test: This PR change exposes a potential issue within the unit test. test_duplicated_duplicated_constant_and_initializer expects optimized model to still contain 2 initializer. But looking at the non_optimized.onnx (in unit_test.zip), only 1 initializer is actually needed. See the optimized_2_inits_left.onnx (in unit_test.zip) for model produced prior to PR change, and optimized_1_inits_left (in unit_test.zip) for model post to PR change. As far as I can see, the later model where only value0 is retained as initializer appear to be what we'd expect.

Please correct me if my understanding on initializer and Const node appear wrong.

repro_issue.zip
unit_test.zip

hwangdeyu
hwangdeyu reviewed Jul 20, 2022
View reviewed changes
Copy link
Contributor

@hwangdeyu hwangdeyu left a comment

Choose a reason for hiding this comment

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

LGTM. Good capture. Thanks!

@q-ycong-p
Copy link
Contributor Author

Thanks @hwangdeyu! When attempting merge, it says "Only those with write access to this repository can merge pull requests". Could you or other contributor of this community merge it?

@hwangdeyu
Copy link
Contributor

hwangdeyu commented Jul 21, 2022 •
edited

Thanks @hwangdeyu! When attempting merge, it says "Only those with write access to this repository can merge pull requests". Could you or other contributor of this community merge it?

Sure☺, I was waitting for CI passed on yesterday, cause I merged latest main into this branch.

@hwangdeyu hwangdeyu merged commit e7f39ed into onnx:main Jul 21, 2022
@q-ycong-p q-ycong-p deleted the fix_const_input branch August 3, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwangdeyu hwangdeyu hwangdeyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@q-ycong-p @hwangdeyu