-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue with importing models using Tensorflow Lite 2.4.x schema #8375
Conversation
Tensorflow Lite has changed the opcode for BuiltinOperators to be represented as 32 bit integers instead of 8 bit integers in the schema. This is an attempt to fix this in a way that is clean to handle multiple versions of tensorflow lite in the frontend.
# lite 2.4.x and hence encase it in a try -except block. | ||
# Phew ! | ||
try: | ||
if op_c.BuiltinCode() < BuiltinOperator.PLACEHOLDER_FOR_GREATER_OP_CODES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are duplicating the implementation. Should not do that. Should always use higher level APIs, so that in future any bug fix or version upgrade, the underlying changes will be taken care automatically.
Use below APIs:
from tensorflow.lite.python import schema_util
from tensorflow.lite.tools import visualize
op_code_list_idx = op.OpcodeIndex()
op_code_id = self.model.OperatorCodes(op_code_list_idx).BuiltinCode()
op_code_id = schema_util.get_builtin_code_from_operator_code(op_code_id)
op_code_str = visualize.BuiltinCodeToName(op_code_id)`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the environment as it would be once the CI images are baked (i.e. with Tensorflow and Tensorflow lite 2.4.2) I cannot seem to find schema_util and visualise packages. Is there something else that you need to do in terms of building tensor flow and tensor flow lite to use this ?
I've tried both:
from tensorflow.lite.python import schema_util
and
from tflite.python import schema_util
Can we fix up the CI breakages with this PR now and follow up with a cleaner fix / cleanup ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Tensorflow 2.3.1 , above APIs are not present, we can have fallback to previous implementation before recent upgrade in TVM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure I've attempted that in the fall back by catching an AttributeError which is what one would get with a missing enum value .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current upgrade to Tensorflow 2.4.2, also does not install schema_util as part of pip package.
SO we can go ahead with current change. Later on when Tensorflow has the bug fixed, we can take the suggested APIs change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll file an issue for it instead of leaving a comment . Further, could you file file a bug in the Tensorflow project or is this a known issue there ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to file bug in Tensorflow.
I have verified my changes in Tf 2.5.0, it works fine.
Hence when we do next Tensorflow version upgrade in TVM. We can incorporate using these Apis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just add a comment that this should be changed to schema_util APIs once Tensorflow has the bug fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work finding a very subtle bug!
Thanks @u99127 @ANSHUMAN87 @leandron |
…pache#8375) Tensorflow Lite has changed the opcode for BuiltinOperators to be represented as 32 bit integers instead of 8 bit integers in the schema. This is an attempt to fix this in a way that is clean to handle multiple versions of tensorflow lite in the frontend.
…pache#8375) Tensorflow Lite has changed the opcode for BuiltinOperators to be represented as 32 bit integers instead of 8 bit integers in the schema. This is an attempt to fix this in a way that is clean to handle multiple versions of tensorflow lite in the frontend.
…pache#8375) Tensorflow Lite has changed the opcode for BuiltinOperators to be represented as 32 bit integers instead of 8 bit integers in the schema. This is an attempt to fix this in a way that is clean to handle multiple versions of tensorflow lite in the frontend.
Tensorflow Lite has changed the opcode for BuiltinOperators
to be represented as 32 bit integers instead of 8 bit integers
in the schema.
This is an attempt to fix this in a way that is clean to handle
multiple versions of tensorflow lite in the frontend.
@ANSHUMAN87 , @mbrookhart