TF Lite unit (kernel) tests CMake build fix #51406
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
madaosik
force-pushed
the
tflite-kernel-tests-cmake-build-fix
branch
from
d31edf9
to
44f505f
Compare
| @@ -29,6 +29,8 @@ constexpr char KernelTestDelegateProviders::kUseSimpleAllocator[]; | |||
|
|
|||
| KernelTestDelegateProviders::KernelTestDelegateProviders() | |||
| : delegate_list_util_(¶ms_) { | |||
| TFLITE_TOOLS_CHECK(delegate_list_util_.GetCount() > 0) | |||
There was a problem hiding this comment.
It's possible that the test could run without any delegate providers. So, this check could fail?
There was a problem hiding this comment.
It is possible to run the test without any delegate provider. It is compulsory, though, to have at least DefaultExecutionProvider instance available acting as a "dummy delegate" providing the common parameters and flags. Without it the test fails during runtime based on the missing "help" parameter (as described here). I admit, nevertheless, that the error message was rather misleading, so I updated it in the latest commit.
|
|
||
| set(DELEGATE_PROVIDERS | ||
| ${DELEGATE_PROVIDERS_SUPP} | ||
| ${TFLITE_SOURCE_DIR}/tools/delegates/coreml_delegate_provider.cc |
There was a problem hiding this comment.
I think some delegate providers are only compilable on certain platforms, e.g., the CoreML delegate might be only compilable for iOS with certain flags as shown by https://github.com/tensorflow/tensorflow/blob/master/tensorflow/lite/tools/delegates/BUILD#L132-L153 when using Bazel. So, we might need additional CMake rules to build these delegate providers?
There was a problem hiding this comment.
right. Also NNAPI delegate should available only for Android
There was a problem hiding this comment.
Good point. Let me put it another way. The tensorflow/lite/CMakeLists.txt provides various options using which the NNAPI, GPU and XNNPACK delegates can be enabled/disabled. As the user can toggle these options even after the configure phase is finished, it is required to have them in the DELEGATE_PROVIDERS list. Conversely and admittedly, the delegates not mentioned in the root CMakeLists (Hexagon, External, GPU) are irrelevant and therefore were removed in the latest commit.
| @@ -157,6 +157,9 @@ class ProvidedDelegateList { | |||
| return CreateAllRankedDelegates(*params_); | |||
| } | |||
|
|
|||
| // Returns the count of referenced delegates in providers_ variable | |||
| const int GetCount(); | |||
There was a problem hiding this comment.
Might simply inline the implementation here? Btw, if the change to tensorflow/lite/kernels/test_delegate_providers.cc were dropped, this function would be no longer needed?
There was a problem hiding this comment.
Implementation has been inlined in the latest commit. The answer is yes. However I would suggest keeping it for the reasons discussed above.
There was a problem hiding this comment.
This overall looks good to me. But I'd like to wait for @terryheo's approval as he is the owner of TFLite CMake support.
| @@ -68,27 +85,34 @@ if(NOT _TFLITE_ENABLE_NNAPI) | |||
| ${TFLITE_SOURCE_DIR}/nnapi/nnapi_util.cc | |||
There was a problem hiding this comment.
Do you need to keep "if(NOT _TFLITE_ENABLE_NNAPI)" here?
|
Merged already as part of #52110. |
The recent changes in the TF Lite kernel tests architecture were reflected in Bazel build system but not in CMake, leading to runtime failure when built using this platform.
This change updates the TF Lite kernel tests CMake build configuration and adds a minor check for easier future debugging.