[Frontend][Torch] Fix up graph input handling by jjohnson-arm · Pull Request #5204 · apache/tvm

jjohnson-arm · 2020-04-01T15:06:50Z

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

From: https://discuss.tvm.ai/t/pytorch-frontend-graph-input-names-can-change-using-loaded-torchscript/6055

Split as two commits to make it easier to review:

Simplify operator input handling
- remove outputs and output_index_map and extend use of input_vars
Allow user supplied input names to override graph inputs
- PyTorch inputs now expected as list rather than dictionary - [(name, shape), (name, shape)...]
- Input names given with from_pytorch() are now set as the graph input names and should be used in set_input()

Review request: @masahi

…nputs

masahi · 2020-04-01T15:56:20Z

@jjohnson-arm need to update the tutorial too.

masahi · 2020-04-01T16:05:15Z

-    for output_name, output in name_output_pairs:
-        output_index_map[output_name] = len(outputs)
-        outputs.append(output)
+def _update_inputs_from_pairs(name_input_pairs, input_vars):


I think we don't need this function anymore. Dict's update method can be used.

masahi · 2020-04-01T16:11:17Z

please update the doc here https://github.com/apache/incubator-tvm/blob/e722301a1c8be3c7052273961b8a408ca5524c76/python/tvm/relay/frontend/pytorch.py#L1434-L1436

We should warn that this names need be around until deployment time. Our suggestion is to choose something obvious, that doesn't require remembering. Something like "input0", "input1" etc

masahi · 2020-04-01T16:14:57Z

I still like to retain the original meaning of input_vars, because this is really relay.Var and represents values that come from outside. outputs is for storing intermediate outputs from preceding relay ops.

I think you can simply replace input_vars with outputs and also inputs with outputs everywhere.

masahi · 2020-04-01T16:19:31Z

+    """
+    input_vars = {}
+    ir_inputs = _get_graph_input_names(graph)
+    for idx, ir_input in enumerate(ir_inputs):


How about

for ir_input, (name, shape) in zip(ir_inputs, input_shapes): ...

masahi · 2020-04-01T16:22:08Z


    params = script_module.state_dict()
-    input_vars = _get_relay_input_vars(input_shapes)
+    input_vars = _get_relay_input_vars(graph, input_shapes)


input_vars -> outputs

masahi · 2020-04-01T16:23:46Z

    """
-    Add quant params to outputs so that they can be referenced by other
+    Add quant params to inputs so that they can be referenced by other
    ops later. Weights are quantized here.


For L104 and L107, please keep outputs

masahi · 2020-04-01T16:24:36Z

-    input_names = [output_index_map[name]
-                   for name in _get_input_names(op_node)]
-    return [outputs[name] for name in input_names]
+def _get_op_inputs(op_node, input_vars):


input_vars -> outputs

Because inputs are not relay.Var.

masahi · 2020-04-01T16:28:32Z

+    input_shapes = list(zip(input_names, ishapes))
+
+    inputs = [torch.randn(shape, dtype=torch.float)
+              for name, shape in input_shapes]


for shape in ishapes

masahi · 2020-04-01T16:29:50Z

cc @alexwong @jwfromm @pyjhzwh This is a API change and will break your code, but for a good reason. See the discussion above.

masahi

Update the tutorial
Update the doc
Remove _update_inputs_from_pairs and use Dict's update method directly everywhere
Minor code style change (for loop)
input_vars -> outputs and inputs -> outputs.

jjohnson-arm · 2020-04-02T07:12:03Z

Update the tutorial

Update the doc

Remove _update_inputs_from_pairs and use Dict's update method directly everywhere

Minor code style change (for loop)

input_vars -> outputs and inputs -> outputs.

Ok - will do. I wasn't sure about the inputs rename, happy to change it back.

jjohnson-arm · 2020-04-02T08:19:15Z

@jjohnson-arm need to update the tutorial too.

Will look at this now.

masahi · 2020-04-02T10:55:44Z

@jjohnson-arm Unfortunately, you've just hit a known flaky test failure. Please comment out the get_valid_count test. See #4901 (comment)

Also have you verified that torch frontend tests work with this PR? I'm not sure some of the usage of update() method is supported.

jjohnson-arm · 2020-04-02T11:05:53Z

@jjohnson-arm Unfortunately, you've just hit a known flaky test failure. Please comment out the get_valid_count test. See #4901 (comment)

Also have you verified that torch frontend tests work with this PR? I'm not sure some of the usage of update() method is supported.

Will comment out the test.
I have been runnning the tests/python/frontend/pytorch/test_forward.py tests for all my changes, and it works fine (using Python 3.6.10). Seems that python from 3.5 has supported tuples etc - https://docs.python.org/3.5/library/stdtypes.html?highlight=update#dict.update, so should be okay?

masahi · 2020-04-02T11:48:12Z

ok good to know. I was thinking the arg of update should be dict.

masahi · 2020-04-02T15:29:55Z

Thanks @jjohnson-arm this is merged!

* [Frontend][Torch] Simplify operator input handling * [Frontend][Torch] Allow user supplied input names to override graph inputs * Fix pylint issues * Updates from code review feedback * Fix tutorial to use shape list input * Disable intermittent test failure in topi vision test

jjohnson-arm added 3 commits April 1, 2020 14:13

[Frontend][Torch] Simplify operator input handling

80984c1

[Frontend][Torch] Allow user supplied input names to override graph i…

de2e6f1

…nputs

Fix pylint issues

48ed1e1

masahi reviewed Apr 1, 2020

View reviewed changes

masahi requested changes Apr 1, 2020

View reviewed changes

Updates from code review feedback

bbe567a

Fix tutorial to use shape list input

fe4331f

Disable intermittent test failure in topi vision test

1adb56a

masahi approved these changes Apr 2, 2020

View reviewed changes

masahi merged commit 03cbf78 into apache:master Apr 2, 2020

ZihengJiang mentioned this pull request Sep 25, 2020

TVM v0.7 Release Note Candidate #6486

Closed

Conversation

jjohnson-arm commented Apr 1, 2020

Uh oh!

masahi commented Apr 1, 2020

Uh oh!

masahi Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

masahi commented Apr 1, 2020

Uh oh!

masahi commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masahi Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

masahi Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

masahi Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

masahi Apr 1, 2020 • 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!

masahi Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

masahi commented Apr 1, 2020

Uh oh!

masahi 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!

jjohnson-arm commented Apr 2, 2020

Uh oh!

jjohnson-arm commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masahi commented Apr 2, 2020

Uh oh!

jjohnson-arm commented Apr 2, 2020

Uh oh!

masahi commented Apr 2, 2020

Uh oh!

masahi commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

masahi commented Apr 1, 2020 •

edited

Loading

masahi Apr 1, 2020 •

edited

Loading

masahi left a comment •

edited

Loading

jjohnson-arm commented Apr 2, 2020 •

edited

Loading