Add Support to Dicts and Strings in ONNX for Inputs and Outputs by lara-hdr · Pull Request #25889 · pytorch/pytorch

lara-hdr · 2019-09-10T02:15:37Z

ONNX does not support dictionaries for inputs and output. The reason is that the arg flattening and unflattening does not handle Dictionary types.
This PR adds flattening/unflattening support for dictionaries and strings.
However this feature should be handled with caution for input dictionaries; and users need to verify their dict inputs carefully, and keep in mind that dynamic lookups are not available.

This PR will allow exporting cases where models have dictionnary outputs (detection and segmentation models in torchvision), and where dictionary inputs are used for model configurations (MultiScaleRoiAlign in torchvision).

lara-hdr · 2019-09-10T02:24:26Z

+      flatten_rec(item.ptr(), args);
+    }
+    structure.push_back(D::DictClose);
+    std::string msg =


This warning prints each time flatten() is called in the code.
We can keep it this way (and have the warning print multiple times in the code), or I an add a boolean arg to flatten() to only print the warnings if the caller is LegacyTracedModule() (and thus print the warning once).

Is the only limitation that the keys must remain the same during the function?

I don't think we should warn at all unless we can confirm that the user is doing something wrong. We don't want to teach people that they should ignore the warnings they see because they are expected. Can we validate after tracing that the dictionary keys are identical?

Yes, that's correct, I will then remove the warnings.

I am not sure to understand your comment "Can we validate after tracing that the dictionary keys are identical?".
you mean by that, that we should verify that the dictionary keys are unchanged during the export?

lara-hdr · 2019-09-12T18:32:19Z

@houseroad, this is required for some pending PRs in torchvision to export the detection models.
cc @BowenBao who reviewed internally.

BowenBao · 2019-09-12T18:45:37Z

@houseroad, this is required for some pending PRs in torchvision to export the detection models.
cc @BowenBao who reviewed internally.

A little more context. The dictionary usages in torchvision detection models is only for model graph configuration, so we are only expecting static usages: dictionary keys are always the same. Likewise how lists are expected as static, and flattened/unflattened, we do the same for dictionaries.

…aidar/support_dict_in_onnx

houseroad · 2019-09-13T19:30:39Z

    std::string msg =
-        "Only tuples, lists and Variables supported as JIT inputs, but got ";
+        "Only tuples, lists and Variables supported as JIT inputs/outputs. "
+        "Dictionaries and strings are also accepted but their usage is not "


@suo does this extension look good to you?

lara-hdr · 2019-09-16T22:45:04Z

@houseroad @suo any updates on this?

lara-hdr · 2019-09-18T04:29:04Z

@houseroad @suo, sorry for the spam, but this PR is blocking pytorch/vision#1324.
Could you please help with the review?

suo · 2019-09-18T06:46:42Z

Sorry for the delay, I'll take a look tomorrow morning

suo · 2019-09-18T21:44:51Z

+      flatten_rec(item.ptr(), args);
+    }
+    structure.push_back(D::DictClose);
+    std::string msg =


Is the only limitation that the keys must remain the same during the function?

I don't think we should warn at all unless we can confirm that the user is doing something wrong. We don't want to teach people that they should ignore the warnings they see because they are expected. Can we validate after tracing that the dictionary keys are identical?

suo · 2019-09-18T21:46:51Z

+    string str = THPUtils_unpackString(obj);
+    args.desc.strings.emplace_back(str);
+    args.desc.structure.push_back(D::String);
+    std::string msg =


same note about warnings here

spandantiwari · 2019-09-19T18:40:12Z

-        "Only tuples, lists and Variables supported as JIT inputs, but got ";
+        "Only tuples, lists and Variables supported as JIT inputs/outputs. "
+        "Dictionaries and strings are also accepted but their usage is not "
+        "recommended. But got unsupported type ";


nit: "But got unsupported type " seems out of place.

lara-hdr · 2019-09-23T20:23:29Z

@suo I added warn_on_static_input_change(), called after tracing, in torch/onnx/utils.py to verify if the dict/str inputs changed, and print the warning in case they are.

Let me know if there are any concerns with the implementation or if you have recommendations for a better design.

houseroad · 2019-09-24T09:16:00Z

@pytorchbot rebase this please

houseroad · 2019-09-24T17:30:28Z

@pytorchbot rebase this please

lara-hdr · 2019-09-24T21:18:12Z

@pytorchbot rebase this please

houseroad

Looks good. Can we also add one test in test_operators.py? It will help us understand the structure when tracing dict.

houseroad · 2019-09-25T00:50:09Z

tests in test_operaters.py keep failing. @lara-hdr could you take a look?

lara-hdr · 2019-09-25T01:02:34Z

@houseroad the errors are happening on the deepcopy() with the error "Only Tensors created explicitly by the user (graph leaves) support the deepcopy protocol at the moment" for the tests in test_operaters.py.

I am waiting on a build rn and will commit a fix soon.

lara-hdr · 2019-09-25T07:04:25Z

@pytorchbot rebase this please

lara-hdr · 2019-09-25T14:59:03Z

@houseroad. failure is unrelated this time

lara-hdr · 2019-09-25T14:59:11Z

@pytorchbot rebase this please

suo

lgtm, thanks for the changes!

lara-hdr · 2019-09-26T22:07:08Z

Thanks a lot @suo!

facebook-github-bot

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

houseroad

overall, looks good. let's land this PR first, and add two more tests in the following PRs. (check my inline comments)

houseroad · 2019-09-27T05:06:32Z

+
+        x = {"test_key_in": torch.randn(1, 2, 3)}
+        self.assertONNX(MyModel(), (x,))
+


Can we have two one test cases:

with string as input,

have some in-place changes, and we get the warning.

facebook-github-bot · 2019-09-27T05:35:12Z

@houseroad merged this pull request in 614edfc.

…rch#25889) Summary: ONNX does not support dictionaries for inputs and output. The reason is that the arg flattening and unflattening does not handle Dictionary types. This PR adds flattening/unflattening support for dictionaries and strings. However this feature should be handled with caution for input dictionaries; and users need to verify their dict inputs carefully, and keep in mind that dynamic lookups are not available. This PR will allow exporting cases where models have dictionnary outputs (detection and segmentation models in torchvision), and where dictionary inputs are used for model configurations (MultiScaleRoiAlign in torchvision). Pull Request resolved: pytorch#25889 Reviewed By: hl475 Differential Revision: D17613605 Pulled By: houseroad fbshipit-source-id: c62da4f35e5dc2aa23a85dfd5e2e11f63e9174db

lara-hdr added 2 commits August 25, 2019 16:01

Add Support to Dicts and Strings in ONNX for Inputs and Outputs

fc69b3b

print warnings

3f071bc

lara-hdr requested a review from apaszke as a code owner September 10, 2019 02:15

pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: onnx Related to torch.onnx module: pybind Related to our Python bindings / interactions with other Python libraries labels Sep 10, 2019

lara-hdr commented Sep 10, 2019

View reviewed changes

lara-hdr mentioned this pull request Sep 11, 2019

Support Exporting MultiScaleRoiAlign to ONNX pytorch/vision#1324

Merged

lara-hdr requested a review from houseroad September 12, 2019 17:41

Merge branch 'master' of https://github.com/lara-hdr/pytorch into lah…

bc3c3a6

…aidar/support_dict_in_onnx

houseroad reviewed Sep 13, 2019

View reviewed changes

houseroad requested a review from suo September 13, 2019 21:38

ezyang added the open source label Sep 18, 2019

suo requested changes Sep 18, 2019

View reviewed changes

spandantiwari reviewed Sep 19, 2019

View reviewed changes

warn only on input change

31235ef

rebase and resolve conflicts

de5d7f5

Merge remote-tracking branch 'origin/master' into HEAD

5f6931c

Merge remote-tracking branch 'origin/master' into HEAD

12a4390

houseroad added this to the 1.3 milestone Sep 24, 2019

Merge remote-tracking branch 'origin/master' into HEAD

299ee4d

houseroad reviewed Sep 24, 2019

View reviewed changes

fix ci failure on tensor deepcopy

991436f

Merge remote-tracking branch 'origin/master' into HEAD

4ab4fc4

Merge remote-tracking branch 'origin/master' into HEAD

9d1667b

yf225 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 25, 2019

suo approved these changes Sep 26, 2019

View reviewed changes

facebook-github-bot reviewed Sep 26, 2019

View reviewed changes

houseroad approved these changes Sep 27, 2019

View reviewed changes

facebook-github-bot closed this in 614edfc Sep 27, 2019

facebook-github-bot added the merged label Sep 27, 2019

lara-hdr mentioned this pull request Oct 15, 2019

JIT inputs does not support collections.OrderedDict #23258

Closed

lara-hdr mentioned this pull request Dec 10, 2019

RuntimeError: Only tuples, lists and Variables supported as JIT inputs, but got dict #13040

Closed

mruberry added the Merged label Oct 28, 2020


		x = {"test_key_in": torch.randn(1, 2, 3)}
		self.assertONNX(MyModel(), (x,))

Conversation

lara-hdr commented Sep 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lara-hdr commented Sep 12, 2019

Uh oh!

BowenBao commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lara-hdr commented Sep 16, 2019

Uh oh!

lara-hdr commented Sep 18, 2019

Uh oh!

suo commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lara-hdr commented Sep 23, 2019

Uh oh!

houseroad commented Sep 24, 2019

Uh oh!

houseroad commented Sep 24, 2019

Uh oh!

lara-hdr commented Sep 24, 2019

Uh oh!

houseroad left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houseroad commented Sep 25, 2019

Uh oh!

lara-hdr commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lara-hdr commented Sep 25, 2019

Uh oh!

lara-hdr commented Sep 25, 2019

Uh oh!

lara-hdr commented Sep 25, 2019

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

lara-hdr commented Sep 26, 2019

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!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

houseroad left a comment •

edited

Loading

lara-hdr commented Sep 25, 2019 •

edited

Loading