POC refactor tflite frontend

[maheshambule]

While working on TFLite Frontend ops, It was observed that most of the common code can be refactored to a single place.
For example below are few redundant things:

• Input and Output tensor checks
• Getting operator options
• Getting Relay expressions for input tensors
• Quantized check
• Fusing activation functions

This PR is a POC to refactor tflite frontend. It adds a decorator which does all the things mentioned above. Currently, this PR demos the decorator for one operator only. I would like to hear opinions from the community about this approach and then once we decide on an approach/design we can refactor all the operators.

This change will allow new developers to quickly add new operators. Plus it will help reviewers also as new devs like me can not make obvious mistakes. :-)

[maheshambule]

@FrozenGene, @anijain2305, @siju-samuel, @u99127, @mbaret Could you please review and let me know your thoughts?

[u99127]

Have you seen #5519 ?

Ramana

[maheshambule]

Have you seen #5519 ?

Ramana

Just now. I replied there.

[u99127]

Have you seen #5519 ?
Ramana

Just now. I replied there.

Yours goes further than #5519 and I'll think through it a bit more. My initial reaction is that this looks ok but there is one aspect that we could pull in here from 5519 :

There is an appeal to keeping the number of inputs and outputs in the table and passing the input and output tensors to the helper functions seems to be high. It also seemed to me that the equality check could be done at the top level and any place where we had a range check we punted to the helper function as I had commented. I'll try and push out something the approach for something like conv2d.

[maheshambule]

I thought of adding all the op attributes in table/map but decorator seemed to be more pythonic.

Few more points to consider:

1. Sometimes equality checks can not be straight forward like num_inputs==input_tensors. In this case we can always set num_input check as None and handle assertion in the specific convert function.
2. Need to check instead of passing input tensors can we pass Relay expressions. If input tensors are needed they can always be accessed using 'op' variable.
3. Need to check if there is a scope to add helper function for quantized params calculation.

[u99127]

Thanks for the discussion and prompting this. I agree with you entirely with the motivation behind this that we should look to simplify the frontend, the amount of copy pasting to add a new operator isn't ideal and reducing the complexity is a good thing.

I thought of adding all the op attributes in table/map but decorator seemed to be more pythonic.

Ok.

Few more points to consider:

1. Sometimes equality checks can not be straight forward like num_inputs==input_tensors. In this case we can always set num_input check as None and handle assertion in the specific convert function.

That seems to be equivalent to -1 in the other scheme . So if we can't handle it in the common case because it has variable number of inputs but greater than a particular number for instance we need to make an operator specific check.

I guess my question is whether a single decorator would be able to handle the most common cases. At the end of the day my goal is to reduce code duplication as much as I can avoid it which is why the table driven approach helps but as you say it has it's limits especially with the cases you mention.

From a very old branch of mine, here is a table with the operator, helper function, number of inputs , number of outputs : all of which are likely to need just an equality check for number of inputs and outputs.

        'AVERAGE_POOL_2D': (self.convert_average_pool2d, 1, 1),
        'SOFTMAX': (self.convert_softmax, 1, 1),
        'SQUEEZE': (self.convert_squeeze, 1, 1),
        'MAX_POOL_2D': (self.convert_max_pool2d, 1, 1),
        'ADD': (self.convert_add, 2, 1),
        'SUB': (self.convert_sub, 2, 1),
        'MUL': (self.convert_mul, 2, 1),
        'DIV': (self.convert_div, 2, 1),
        'POW': (self.convert_pow, 2, 1),
        'MAXIMUM': (self.convert_maximum, 2, 1),
        'MINIMUM': (self.convert_minimum, 2, 1),
        'GREATER': (self.convert_greater, 2, 1),
        'LOGISTIC': (self.convert_logistic, 1, 1),
        'TANH':(self.convert_tanh, 1, 1),
        'RELU':(self.convert_relu, 1, 1),
        'CAST': (self.convert_cast, 1, 1),
        'TRANSPOSE_CONV': (self.convert_transpose_conv, 3, 1)

as these have fixed number of input and output tensors.

My hunch is that we should be able to get to a single decorator for this sample set above falls out but I'd like to see what you think. Without working it out

If on the other hand we end up with multiple decorators per helper for each of these , then we end up with duplication of code again and copy-pastism which I think is what we both want to avoid.

2. Need to check instead of passing input tensors can we pass Relay expressions. If input tensors are needed they can always be accessed using 'op' variable.

What would the relay expressions represent ?

3. Need to check if there is a scope to add helper function for quantized params calculation.

I'm not sure I follow this one.

[maheshambule]

Thanks for the response.

Few more points to consider:

Let me clarify, the points which I mentioned are not for in or against both the approaches (table vs decorator). They were for general discussion.

I think as far as code deduplication is considered both the approaches are fine. Both can achieve the same. The difference comes with the look and feel of it. And hence it is subjective and can be driven by personal choices. However, still, let me put a few pros of a decorator approach.

• Since it is more pythonic, new developers could easily relate to it.
• The decorator places all the attributes/properties of a particular op in a single view. For table based approach these are divided into two places - at table level and in the convert function.
• The decorator intuitively lets you add pre and post-processing. For. ex. fusing activation functions to the output.

Let me know your thoughts.

That seems to be equivalent to -1 in the other scheme

Ok. Need to decide -1 or None.

My hunch is that we should be able to get to a single decorator for this sample set above falls out but I'd like to see what you think. Without working it out

I think a single decorator will suffice.

What would the relay expressions represent ?

There is a common pattern where we convert TFLite tensor expression to Relay expression often using self.get_expr. Should we push back this conversion to a common code? Also a more generic implementation of conversion is added in this PR as get_tensor_expr function https://github.com/apache/incubator-tvm/pull/5528/files#diff-ee9a43edfb6fd0c9121e00c67c59ef43R2414.

I'm not sure I follow this one.

I was thinking if we could somehow find some common code that will be a wrapper to code like below. But on the second though, I think it is not worth the effort.
https://github.com/apache/incubator-tvm/blob/e2bd43b6f728d4d0ca2659dcf73da74294655133/python/tvm/relay/frontend/tflite.py#L633-L641

[anijain2305]

@u99127 @maheshambule Are we following up on this?

[maheshambule]

@anijain2305, it would be great to hear your feedback/review on this proposed refactoring.
@u99127 Do you want to add further comments/suggestions here.

[tqchen]

@anijain2305 @u99127 please followup :)

[jroesch]

I am doing triage on old PRs, going to close this, please feel free to follow up if you would like to still merge these changes. Thanks for your contributions!.