Retain the parameter names in ONNX exporter #17551
Conversation
houseroad
force-pushed
the
onnx_param_name
branch
from
43910a1 to
481aa30
Compare
torch/onnx/utils.py
Outdated
There was a problem hiding this comment.
A minor point - could we consider another name for this parameter, such as retain_param_name or export_with_param_name?
houseroad
force-pushed
the
onnx_param_name
branch
from
481aa30 to
d2465c7
Compare
houseroad
changed the title
dzhulgakov
left a comment
dzhulgakov
left a comment
There was a problem hiding this comment.
Awesome. Why not make it a default right away?
torch/onnx/utils.py
Outdated
There was a problem hiding this comment.
what happens if there's a duplicated name?
There was a problem hiding this comment.
https://github.com/pytorch/pytorch/blob/master/torch/csrc/jit/ir.cpp#L670, it will rename the old owner of this name. We should be good. :-)
bddppq
left a comment
bddppq
left a comment
There was a problem hiding this comment.
I suggest let's always enable it (instead of having a flag to control)
torch/onnx/utils.py
Outdated
There was a problem hiding this comment.
nit:
for i, inp in enumerate(graph_inputs):
inp.setUniqueName(...)
torch/onnx/utils.py
Outdated
There was a problem hiding this comment.
wait, the order of the keys is not stable
|
I will rebase this pr, and have another PR to enable it as default behavior. Don't want to let the massive updated expect files overwhelmed the important changes. I think to be safe, let's at least keep this flag for now. |
houseroad
force-pushed
the
onnx_param_name
branch
from
d2465c7 to
eba0a02
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
If you plan this flag as a temporary thing - start the name of the argument with underscore (i.e. _preserve_param_names) |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Can you please revert all of the changed |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
dzhulgakov
left a comment
dzhulgakov
left a comment
There was a problem hiding this comment.
ship it
@sampepose - the .expect file changes are expected as we change the names of the parameters
| graph_inputs = list(graph.inputs()) | ||
| user_input_num = len(graph_inputs) - len(state_dict) | ||
| param_names = list(state_dict.keys()) | ||
| for i, inp in enumerate(graph_inputs): |
There was a problem hiding this comment.
or for inp, name in zip(graph_inputs[user_input_num:], param_names):
7 participants
So, we will keep the names of ONNX initializers the same as the names in PyTorch state dict.
Later, we will make this as the default behavior.