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

[TFLite] Quantization and unfolding of the ADD_N operator #44806

Open
Tessil opened this issue Nov 12, 2020 · 17 comments
Open

[TFLite] Quantization and unfolding of the ADD_N operator #44806

Tessil opened this issue Nov 12, 2020 · 17 comments
Assignees
Labels
comp:lite TF Lite related issues ModelOptimizationToolkit TF Model Optimization Toolkit stat:awaiting tensorflower Status - Awaiting response from tensorflower TFLiteConverter For issues related to TFLite converter type:feature Feature requests

Comments

@Tessil
Copy link
Contributor

Tessil commented Nov 12, 2020

Hello,

When adding multiple tensors (> 2) in a NN we can either use tf.keras.layers.Add or tf.math.add_n. The first one is quantizable but not the second one as unfortunatly even if both have the same high-level functionality they aren't converted in the same way when using the TFLiteConverter.

The tf.keras.layers.Add layer is converted to a cascade of binary ADD operators:
add_cascade

The tf.math.add_n on the other hand is converted to a single not yet quantizabe ADD_N operator:
add_n

It seems there is an existing LowerAddNOp transformation to lower the ADD_N operator into an adder tree of binary ADD operators (instead of a cascade) but for some reasons it doesn't seem to apply during the tests I did.

As both functions provide the same functionality it would be ideal for them to be exported in the same way in TFLite to avoid any output difference which could occur due to different kernel implementations and additions order as the floating-point and quantized additions are not associative. Unfolding the ADD_N operator would also allow to easily reuse the optimized ADD kernels (both for HW and SW kernels).

As we would like to add support for the quantization of models with tf.math.add_n I was wondering what were the plans regarding the ADD_N kernel. Would it be alright to unfold it into a cascade of adds? Or was the plan to unfold it into an adder tree (which would then produce different results than tf.keras.layers.Add as the order of additions would not be the same)?

Thanks,
Thibaut

@Tessil Tessil added the comp:lite TF Lite related issues label Nov 12, 2020
@amahendrakar amahendrakar added the type:support Support issues label Nov 13, 2020
@amahendrakar amahendrakar assigned ymodak and unassigned ravikyram Nov 13, 2020
@ymodak ymodak assigned smit-hinsu and unassigned ymodak Nov 14, 2020
@ymodak ymodak added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Nov 14, 2020
@wangtz wangtz added the TFLiteConverter For issues related to TFLite converter label Nov 16, 2020
@smit-hinsu smit-hinsu assigned abattery and unassigned smit-hinsu Nov 16, 2020
@abattery
Copy link
Contributor

abattery commented Nov 16, 2020

This is a good example of quantization aware conversion. FYI, @karimnosseir @renjie-liu

@karimnosseir
Copy link
Contributor

@Tessil We should be doing the TF Lowering pass and you should see the add Tree. Did you try converting using the nightly build ?

@abattery
Copy link
Contributor

abattery commented Nov 17, 2020

I think that is the expected behavior. The TF Lower pass will be trigged only when the given TensorFlow core operators are not directly legalized. Since we have the ADD_N op implementation, the legalization procedure prefers adding the the TFLite ADD_N op into the flatbuffer over the ADD op tree addition.

If we would do that always, we need to add the the ADD_N op --> ADD op tree lowering rule in the early TensorFlow preparation stage.

@karimnosseir
Copy link
Contributor

Thanks for checking @abattery. @Tessil we are looking in adding ways to easily customize passes/patterns during conversion. Stay tuned.

@Tessil
Copy link
Contributor Author

Tessil commented Nov 17, 2020

Thank you very much for your answers. I did my tests with the nightly build.

Note that if we lower the ADD_N to a series of ADD, it would be nice to keep some coherence with the lowering done in tf.keras.layers.Add to avoid numerical differences between the two operators (either an adder-tree for both or a cascade for both) .

@tensorflowbutler tensorflowbutler removed the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Nov 19, 2020
@Tessil
Copy link
Contributor Author

Tessil commented Apr 13, 2021

Hi @abattery @karimnosseir I was wondering if there was any eventual news on the issue? Should we aim to lower the tf.math.add_n operator to a series of add similar to tf.keras.layers.Add by default or should we keep the AddN operator (though in this case the quantized versions of both operators may produce different results and lack coherence)?

@abattery
Copy link
Contributor

For the workaround, maybe you create a custom keras layer to meet your needs.

@Tessil
Copy link
Contributor Author

Tessil commented Apr 13, 2021

For now we can just replace tf.math.add_n with tf.keras.layers.Add as temporary workaround but we're mainly interested by resolving the incoherence between the two operators to make it easier to support both operators on HW platforms.

@abattery abattery added ModelOptimizationToolkit TF Model Optimization Toolkit and removed TFLiteConverter For issues related to TFLite converter labels Apr 13, 2021
@abattery
Copy link
Contributor

@teijeong could you triage this issue?

@abattery
Copy link
Contributor

FYI, @jianlijianli

@abattery abattery removed their assignment Apr 13, 2021
@akarmi
Copy link
Contributor

akarmi commented Apr 16, 2021

Hi @abattery, out of interest, what is the difference between "ModelOptimizationTookit" and "TFLiteConverter" labels? I noticed you re-assigned this issues from the latter to the former above. Quick check of other issues under both seems to suggest that both are applicable to issues with the TFLite Converter.

@abattery
Copy link
Contributor

TFLite converter is a gateway to the TFLite product. However, it contains several components, TF -> TFLite graph lowering and model optimization techniques. This issue is related to the model optimizations so to be clear, we tried to set the model optimization tag for such cases.

@Saduf2019
Copy link
Contributor

@Tessil
Is this still an issue

@Saduf2019 Saduf2019 added the stat:awaiting response Status - Awaiting response from author label Nov 2, 2021
@Tessil
Copy link
Contributor Author

Tessil commented Nov 2, 2021

Hi, yes tf.keras.layers.Add and tf.math.add_n still behave differently and can produce different quantized results.

@Saduf2019 Saduf2019 removed the stat:awaiting response Status - Awaiting response from author label Nov 2, 2021
@pkgoogle pkgoogle self-assigned this Jul 19, 2023
@pkgoogle pkgoogle added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Jul 19, 2023
@pkgoogle pkgoogle added type:feature Feature requests type:support Support issues and removed type:support Support issues type:feature Feature requests labels Aug 14, 2023
@pjpratik pjpratik self-assigned this Aug 22, 2023
@pjpratik
Copy link
Contributor

Hi,

Thank you for opening this issue. Since this issue has been open for a long time, the code/debug information for this issue may not be relevant with the current state of the code base.

The TFLite team is constantly improving the framework by fixing bugs and adding new features. We suggest you try the latest TensorFlow version with the latest compatible hardware configuration which could potentially resolve the issue. If you are still facing the issue, please create a new GitHub issue with your latest findings, with all the debugging information which could help us investigate.

Please follow the release notes to stay up to date with the latest developments which are happening in the TFLite space.

Thanks.

@pjpratik pjpratik added stat:awaiting response Status - Awaiting response from author and removed stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Aug 22, 2023
@github-actions
Copy link

This issue is stale because it has been open for 7 days with no activity. It will be closed if no further activity occurs. Thank you.

@github-actions github-actions bot added the stale This label marks the issue/pr stale - to be closed automatically if no activity label Aug 30, 2023
@Tessil
Copy link
Contributor Author

Tessil commented Aug 30, 2023

Hi,

The issue is still present in the latest 2.13 version of TF.

@google-ml-butler google-ml-butler bot removed stale This label marks the issue/pr stale - to be closed automatically if no activity stat:awaiting response Status - Awaiting response from author labels Aug 30, 2023
@pkgoogle pkgoogle added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Aug 30, 2023
@pkgoogle pkgoogle added type:bug Bug type:feature Feature requests TFLiteConverter For issues related to TFLite converter and removed type:support Support issues type:bug Bug labels Sep 7, 2023
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 ModelOptimizationToolkit TF Model Optimization Toolkit stat:awaiting tensorflower Status - Awaiting response from tensorflower TFLiteConverter For issues related to TFLite converter type:feature Feature requests
Projects
None yet
Development

No branches or pull requests