[ONNX] Refactoring serialization of ONNX initializers to be name-based (Resubmission) #17830

spandantiwari · 2019-03-09T01:16:30Z

@houseroad - this is the resubmission of #17420, as suggested.

houseroad · 2019-03-09T06:13:53Z

Apparently, this break several internal pipelines... I need more time to investigate what's going on.

houseroad

Put it on hold before the internal tests pass.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

spandantiwari · 2019-03-10T08:13:15Z

@houseroad - OK. Could it be because of the order the initializers are written to the ONNX file, because that may change for some graphs with this change? But that should be a one-time update to the expect file.
Let me know if I can help.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad

The problem is only triggered when we set input_names and using non-protobuf serialization format (e.g., ZIP_ARCHIVE). We should add a test case to guard such situation.

torch/onnx/utils.py

spandantiwari · 2019-03-27T00:10:52Z

Fixed the issue with setting input_names and using non-protobuf serialization format (e.g., ZIP_ARCHIVE). Also added a test for guarding against that.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

torch/onnx/utils.py

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad

Let's add a test case to cover the input_names conflict with existing parameter names.

Something like model has 2 parameters and 1 input.
export(model, input_names=['input1', 'p2']) and the second parameter's name is p2, and it will be renamed to p2.1 after exporting to onnx.

torch/onnx/utils.py

spandantiwari · 2019-03-28T21:45:42Z

Moved the params_dict creation code to the end as suggested. Added a new test point that checks tests the deliberate parameter name clobber.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

spandantiwari · 2019-03-29T04:21:36Z

@houseroad - the failures seem unrelated. I have seen some of them in previous runs as well.

houseroad

Looks good! Finally, we fixed all the problems.

Left some nits.

test/onnx/test_pytorch_onnx_caffe2.py

torch/csrc/jit/python_ir.cpp

…es and not order.

…alization of initializers.

spandantiwari · 2019-03-29T18:02:40Z

@houseroad - Great to hear that all tests are passing. Thanks for your tremendous help!
I have updated the PR based on the feedback.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-03-30T01:05:06Z

@houseroad merged this pull request in 1240327.

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 9, 2019

houseroad requested changes Mar 9, 2019

View reviewed changes

facebook-github-bot reviewed Mar 9, 2019

View reviewed changes

spandantiwari force-pushed the spandantiwari/params_with_names branch from 6b45432 to f48e2ec Compare March 16, 2019 00:27

facebook-github-bot reviewed Mar 18, 2019

View reviewed changes

houseroad reviewed Mar 26, 2019

View reviewed changes

torch/onnx/utils.py Outdated Show resolved Hide resolved

spandantiwari force-pushed the spandantiwari/params_with_names branch from f48e2ec to fb28b81 Compare March 27, 2019 00:09

facebook-github-bot reviewed Mar 27, 2019

View reviewed changes

spandantiwari force-pushed the spandantiwari/params_with_names branch from 15ac29e to 451894e Compare March 27, 2019 18:52

houseroad reviewed Mar 28, 2019

View reviewed changes

torch/onnx/utils.py Outdated Show resolved Hide resolved

spandantiwari force-pushed the spandantiwari/params_with_names branch from 451894e to 1a8220b Compare March 28, 2019 00:58

facebook-github-bot reviewed Mar 28, 2019

View reviewed changes

houseroad reviewed Mar 28, 2019

View reviewed changes

torch/onnx/utils.py Outdated Show resolved Hide resolved

torch/onnx/utils.py Outdated Show resolved Hide resolved

torch/onnx/utils.py Outdated Show resolved Hide resolved

spandantiwari force-pushed the spandantiwari/params_with_names branch from 1a8220b to d5f87f5 Compare March 28, 2019 21:43

spandantiwari force-pushed the spandantiwari/params_with_names branch from d5f87f5 to bf4ae98 Compare March 28, 2019 22:28

facebook-github-bot reviewed Mar 28, 2019

View reviewed changes

houseroad approved these changes Mar 29, 2019

View reviewed changes

Spandan Tiwari added 7 commits March 29, 2019 10:17

Onnx export - refactoring to serialize ONNX initializers based on nam…

dbffa44

…es and not order.

Incorporate review feedback.

b9acad3

Fixing how empty dict is passed in ONNX export.

a17a3ea

Fixing default argument style for unordered_map to fix test failures.

938b766

Style fixes to assuage flake8.

7376cf5

Updating two ONNX test expect files.

0b5b2cc

Always writing initializers to ONNX in the same order.

75e04dd

Spandan Tiwari added 7 commits March 29, 2019 10:17

Updating export method to use std::map for determinstic order of seri…

78e2d05

…alization of initializers.

Fixed issue related to parameter renaming during ONNX export.

881fadf

Fixing lint issues.

e2197dc

Updating expect files for tests whose initializer order has changed.

6528065

Moving params_dict generation to after _optimize_graph().

592477d

Moving params_dict creation to the end of _model_to_graph.

bfa8180

Cleaning up import statements.

13c5516

spandantiwari force-pushed the spandantiwari/params_with_names branch from bf4ae98 to 13c5516 Compare March 29, 2019 18:00

facebook-github-bot reviewed Mar 29, 2019

View reviewed changes

facebook-github-bot closed this in 1240327 Mar 29, 2019

facebook-github-bot added the merged label Mar 30, 2019

ezyang added the open source label Jun 24, 2019

[ONNX] Refactoring serialization of ONNX initializers to be name-based (Resubmission) #17830

[ONNX] Refactoring serialization of ONNX initializers to be name-based (Resubmission) #17830

Uh oh!

Conversation

spandantiwari commented Mar 9, 2019

Uh oh!

houseroad commented Mar 9, 2019

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

spandantiwari commented Mar 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spandantiwari commented Mar 27, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spandantiwari commented Mar 28, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

spandantiwari commented Mar 29, 2019

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spandantiwari commented Mar 29, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spandantiwari commented Mar 10, 2019 •

edited

Loading