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

[ONNX] Set node.target instead of node.name as ONNX initializer name #104670

Closed
BowenBao opened this issue Jul 5, 2023 · 0 comments
Closed

[ONNX] Set node.target instead of node.name as ONNX initializer name #104670

BowenBao opened this issue Jul 5, 2023 · 0 comments
Labels
module: onnx Related to torch.onnx triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@BowenBao
Copy link
Collaborator

BowenBao commented Jul 5, 2023 

          Actually I cannot do this in this PR. A bug is revealed that we are naming ONNX initializer by node.name instead of node.target. It has two issues
  1. Duplicated initializers if multiple getattr nodes were called to retrieve same parameter.
  2. node.name is not always the same as node.target. Previously this is left unnoticed. Uncovered here due to that '/' is not allowed in node.name, but allowed in node.target. This means we still get _ in the onnx model initializer names in the end, even if we set to / here.

My plan is to merge this PR with _ to stay consistent, and follow up another PR to fix initializer name, and revert to / there.

Originally posted by @BowenBao in #104493 (comment)
@BowenBao BowenBao added the module: onnx Related to torch.onnx label Jul 5, 2023
@drisspg drisspg added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 5, 2023
@BowenBao BowenBao mentioned this issue Jul 6, 2023
Closed
@thiagocrepaldi thiagocrepaldi mentioned this issue Jul 7, 2023
Closed
BowenBao added a commit that referenced this issue Jul 7, 2023
@BowenBao
Update base for Update on "[ONNX] Fix exported onnx initializer name"
08b74c6 
Restore ONNX initializer name to exactly match torch parameter and buffer name.
Fixes #104670 that could lead to potentially duplicated ONNX initializers after export.

[ghstack-poisoned]
BowenBao added a commit that referenced this issue Jul 7, 2023
@BowenBao
Update on "[ONNX] Fix exported onnx initializer name"
61a454b 
Restore ONNX initializer name to exactly match torch parameter and buffer name.
Fixes #104670 that could lead to potentially duplicated ONNX initializers after export.

[ghstack-poisoned]
BowenBao added a commit that referenced this issue Jul 7, 2023
@BowenBao
Update base for remove dup check on "[ONNX] Fix exported onnx initial…
1e0947d 
…izer name"


Restore ONNX initializer name to exactly match torch parameter and buffer name.
Fixes #104670 that could lead to potentially duplicated ONNX initializers after export.

[ghstack-poisoned]
BowenBao added a commit that referenced this issue Jul 7, 2023
@BowenBao
remove dup check on "[ONNX] Fix exported onnx initializer name"
19088d1 
Restore ONNX initializer name to exactly match torch parameter and buffer name.
Fixes #104670 that could lead to potentially duplicated ONNX initializers after export.

[ghstack-poisoned]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
module: onnx Related to torch.onnx triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
ONNX
Status: Done
Milestone
No milestone
Development

No branches or pull requests
2 participants
@BowenBao @drisspg