[Relay] Remove DynamicToStatic pass from graph runtime build #10691
Conversation
|
I realized that this is not specific to meta scheduler (the one I tested), but the same error could happen for auto scheduler as well since it also relies on the structure of the input relay subgraph for a workload lookup. All tests have passed without So I believe it is better to remove |
| @@ -846,7 +846,7 @@ def convert_shape(self, op): | |||
| input_tensors = self.get_input_tensors(op) | |||
| assert len(input_tensors) == 1, "input tensors length should be 1" | |||
|
|
|||
| out = _op.shape_of(self.get_tensor_expr(input_tensors[0])) | |||
| out = shape_of(self.get_tensor_expr(input_tensors[0])) | |||
There was a problem hiding this comment.
Before this PR, tflite mobilenet was returning a dynamic shape output 🤦♂️
| pass_seqs.push_back(transform::CombineParallelConv2D(3)); | ||
| pass_seqs.push_back(transform::CombineParallelDense(3)); | ||
| pass_seqs.push_back(transform::CombineParallelBatchMatmul(3)); | ||
| pass_seqs.push_back(transform::FoldConstant()); | ||
| pass_seqs.push_back(transform::FoldScaleAxis()); | ||
| pass_seqs.push_back(transform::SimplifyExpr()); |
There was a problem hiding this comment.
I realized that SimplifyExpr practically depends on FoldConstant be applied beforehand. So I swapped the order.
| if default_value == None: | ||
| output = tf.sparse_to_dense(indices, oshape, values) | ||
| compare_tf_with_tvm( | ||
| [sparse_indices, sparse_values], ["indices:0", "values:0"], output.name | ||
| [sparse_indices, sparse_values], ["indices:0", "values:0"], output.name, mode="vm" |
There was a problem hiding this comment.
This test was previously working on the graph runtime just because the dynamic input is bound to a constant tensor before relay.build(...) in TF tests. Such usage of dynamic inputs makes no sense so I just changed it to use vm for testing.
| np_data, | ||
| "", | ||
| ["UniqueWithCounts:0", "UniqueWithCounts:1", "UniqueWithCounts:2"], | ||
| mode="vm", |
There was a problem hiding this comment.
Unique is naturally a dynamic op, but for some reason this test was running on the graph runtime and it happens to be working just because the dynamic input is bound to a constant tensor before relay.build(...). So the test was effectively running unique(const_tensor), which is not really useful.
|
I am in favor of remove DynamicToStatic from the default set of passes. |
…10691) Closes apache#10692 To solve this problem, we can either remove this pass from `relay.build(...)` pipeline or run `DynamicToStatic` in both VM and non-VM paths. I propose to remove it because (1) usually `DynamicToStatic` is supposed to be applied after model import and (2) the only case running `DynamicToStatic` during `relay.build(...)` helps is when the input is entirely static but a frontend fails to produce a static mod AND a user forgets to run `DynamicToStatic` after model import. I hope the latter case happens rarely but if not, that's something we should fix in the frontend side. We should avoid relying on `DynamicToStatic` that runs during `relay.build(...)` since not all use cases of TVM use `relay.build(...)` (BYOC, for example).
Closes #10692
To solve this problem, we can either remove this pass from
relay.build(...)pipeline or runDynamicToStaticin both VM and non-VM paths. I propose to remove it because (1) usuallyDynamicToStaticis supposed to be applied after model import and (2) the only case runningDynamicToStaticduringrelay.build(...)helps is when the input is entirely static but a frontend fails to produce a static mod AND a user forgets to runDynamicToStaticafter model import.I hope the latter case happens rarely but if not, that's something we should fix in the frontend side. We should avoid relying on
DynamicToStaticthat runs duringrelay.build(...)since not all use cases of TVM userelay.build(...)(BYOC, for example).