Skip to content
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

TensorFlow Lite Metadata duplication #39109

Closed
lutzroeder opened this issue May 3, 2020 · 5 comments
Closed

TensorFlow Lite Metadata duplication #39109

lutzroeder opened this issue May 3, 2020 · 5 comments
Assignees
Labels
comp:lite TF Lite related issues type:feature Feature requests

Comments

@lutzroeder
Copy link
Contributor

lutzroeder commented May 3, 2020

@lu-wang-g @petewarden @aselle

ModelMetadata defines another description property when one is already defined on Model.

Would it make sense to encode model level properties like author, name, license, version as Metadata properties on the model itself instead of defining them in ModelMetadata. That way all TensorFlow Lite models could carry such information as part of the model without relying on model_schema.fbs for decoding (similar to metadata_props in ONNX).

See lutzroeder/netron#481

@lutzroeder lutzroeder added the type:bug Bug label May 3, 2020
@lutzroeder lutzroeder changed the title Duplication between metadata_schema.fbs and schema.fbs TensorFlow Lite Metadata duplication May 3, 2020
@Saduf2019 Saduf2019 added type:feature Feature requests comp:lite TF Lite related issues and removed type:bug Bug labels May 3, 2020
@Saduf2019 Saduf2019 assigned ymodak and unassigned Saduf2019 May 3, 2020
@ziyeqinghan
Copy link
Contributor

@lu-wang-g Could you please take a look?

@lu-wang-g
Copy link
Member

@lutzroeder ModelMetadata is a centralized place to hold all metadata of the model. It is easier for users to get the information at once by parsing ModelMetadata. Since TFLite metadata is still at experimental, we didn't mix it with the existing fields in the TFLite schema. It is a good point that we should remove the duplicated fields in the TFLite schema once metadata_schema.fbs is mature.

Seems like you had a protptype to show TFLite metadata in Netron. It will be super help for the metadata use cases. Thanks for working on it. Let me know if you have any questions when implementing the feature. Thanks!

@ymodak ymodak assigned lu-wang-g and unassigned ymodak May 4, 2020
@ymodak ymodak added the stat:awaiting response Status - Awaiting response from author label May 4, 2020
@lutzroeder
Copy link
Contributor Author

lutzroeder commented May 4, 2020

@lu-wang-g

It is easier for users to get the information at once by parsing ModelMetadata.

Is that true? Since it's hidden inside the metadata of the .tflite file and requires a separate schema there is more complexity involved getting to this information. Especially for top-level model properties like author, description and license why require another standard if they can be added as (string, byte[]) to the model itself without changing the schema - isn't that the purpose of the metadata table in .tflite?

It is a good point that we should remove the duplicated fields in the TFLite schema once metadata_schema.fbs is mature.

Will the files that exist now continue to work if the schema keeps changing or will files that get created with the experimental feature right now be in an invalid format in the future?

Having two separate model descriptions is problematic right now. Which one should a tool show? The one in the .tflite file or the one in TFLITE_METADATA? Showing both is confusing as well...

Seems like you had a protptype to show TFLite metadata in Netron. It will be super help for the metadata use cases.

Netron 4.1.4 will show the metadata if present.

screenshot

Was there a reason the metadata variable was called TFLITE_METADATA upper case instead of using lower case like other metadata property names? TFLITE seems redundant?

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label May 6, 2020
@lu-wang-g
Copy link
Member

@lutzroeder

Is that true? Since it's hidden inside the metadata of the .tflite file and requires a separate schema there is more complexity involved getting to this information. Especially for top-level model properties like author, description and license why require another standard if they can be added as (string, byte[]) to the model itself without changing the schema - isn't that the purpose of the metadata table in .tflite?

The metadata can be extracted through the metadata extractor library. We've launched the Java version, and the C++ library will come soon. Users can also convert the metadata into a Json file through the metadata displayor. Where the metadata is located in the model should be transparent for users.

The metadata table in .tflite is not designed for the information like TFLite metadata, but mainly used as buffers of internal data that is supposed to be hidden from users. Having users editing against it directly is error prone, because the metadata table accepts data in any types and any formats in general.

Will the files that exist now continue to work if the schema keeps changing or will files that get created with the experimental feature right now be in an invalid format in the future?

Having two separate model descriptions is problematic right now. Which one should a tool show? The one in the .tflite file or the one in TFLITE_METADATA? Showing both is confusing as well...

Up to now, we're pretty confident about the Metadata schema and the fields it defines. It works well on common image models such as classification and object detection. It will be moved out from experimental soon. Regarding my previous comment of deprecating certain fields, it was an impression at first sight, and it's mainly just about the description field of the model. I agree it's quite confusing to have two model description fields, and I think the one in .tflite is not used that much so that should be OK to deprecate it. But we'll need to further evaluate if it makes sense to do so.

screenshot

The UI looks so cool! Really appreciate your efforts for support TFLite Metadata in Netron!

Was there a reason the metadata variable was called TFLITE_METADATA upper case instead of using lower case like other metadata property names? TFLITE seems redundant?

It's just a token to identify the TFLite Metadata, and to be differentiated from other metadata entries in the metadata table. Again, it is supposed to be transparent to users, a nuance that hopefully won't be noticed by them.

@lu-wang-g
Copy link
Member

Closing this issue for now. Feel free to reopen it if you have another questions. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:lite TF Lite related issues type:feature Feature requests
Projects
None yet
Development

No branches or pull requests

6 participants