Skip to content

Conversation

sl1g18
Copy link

@sl1g18 sl1g18 commented Apr 29, 2021

Contribution description

This PR updates the tensorflow-lite pkg version to v2.2.2. This was done to support new operations and optimization features available in newer versions of tflite. Flatbuffers was updated to v1.12 and gemmlowp:09.2018 to ensure compatibility.

Testing procedure

I ran the pkg_tensorflow-lite test with the updated packages on the samr21-xpro. This returned the nominal output: 'Digit prediction: 7'

Issues/PRs references

@benpicco benpicco added Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Apr 29, 2021
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you for taking care of this!
We should really try to push #13911 forward.

Just one thing: please make sure commit messages are < 72 characters so they are not trunctaed & CI is happy.

pkg/tensorflow_lite: Update to v2.2.2 

is entirely sufficient.

For gemmlowp you forgot the pkg/gemmlowp prefix in the commit message.

@@ -1,6 +1,6 @@
PKG_NAME=flatbuffers
PKG_URL=https://github.com/google/flatbuffers
PKG_VERSION=v1.11.0
PKG_VERSION=6df40a2471737b27271bdd9b900ab5f3aec746c7
Copy link
Contributor

@benpicco benpicco Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PKG_VERSION=6df40a2471737b27271bdd9b900ab5f3aec746c7
PKG_VERSION=6df40a2471737b27271bdd9b900ab5f3aec746c7 # 1.12

You can also use tags instead of the commit hash if those are available.

@@ -1,6 +1,6 @@
PKG_NAME=tensorflow-lite
PKG_URL=https://github.com/tensorflow/tensorflow
PKG_VERSION=1768c8f2fa155d4c6406e8ff7addf374c83de7ad
PKG_VERSION=d745ff2a48cebf18e847e8b602a744e97e058946
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PKG_VERSION=d745ff2a48cebf18e847e8b602a744e97e058946
PKG_VERSION=d745ff2a48cebf18e847e8b602a744e97e058946 # 2.2.2

@@ -1,6 +1,6 @@
PKG_NAME=gemmlowp
PKG_URL=https://github.com/google/gemmlowp
PKG_VERSION=dc69acdf61d7a64260ae0eb9c17421fef0488c02
PKG_VERSION=719139ce755a0f31cbf1c37f7f98adcc7fc9f425
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PKG_VERSION=719139ce755a0f31cbf1c37f7f98adcc7fc9f425
PKG_VERSION=719139ce755a0f31cbf1c37f7f98adcc7fc9f425 # 09.2018

@sl1g18
Copy link
Author

sl1g18 commented Apr 29, 2021

Great, thank you for taking care of this!
We should really try to push #13911 forward.

Just one thing: please make sure commit messages are < 72 characters so they are not trunctaed & CI is happy.

pkg/tensorflow_lite: Update to v2.2.2 

is entirely sufficient.

For gemmlowp you forgot the pkg/gemmlowp prefix in the commit message.

Thank you! This was my first pull request so wasn't sure what to expect! Should I edit the commit messages to remedy the omission/shorten the messages?

Best regards,
Sergiu

@benpicco
Copy link
Contributor

Yes just run git rebase -i HEAD~3 and select 'reword' to edit the commit messages.

@benpicco
Copy link
Contributor

benpicco commented Apr 29, 2021

Looks like the build fails now on RISC-V. (BOARD=hifive1)

@sl1g18
Copy link
Author

sl1g18 commented Apr 29, 2021

Looks like the build fails now on RISC-V. (BOARD=hifive1)

Very interesting, I'm wondering where the board builds diverge and why in this case?

@aabadie
Copy link
Contributor

aabadie commented Apr 29, 2021

Thanks for trying to update tensorflow-lite package.

Regarding the failure on RISC-V, you have to add a patch file similar to the existing one, but applied to "tensorflow/lite/kernels/internal/reference/reduce.h" and it seems another one is needed for "tensorflow/lite/micro/kernels/activation_utils.h" with fmin/fmax functions but I don't know how to fix it.

@sl1g18
Copy link
Author

sl1g18 commented Apr 29, 2021

Thanks for trying to update tensorflow-lite package.

Regarding the failure on RISC-V, you have to add a patch file similar to the existing one, but applied to "tensorflow/lite/kernels/internal/reference/reduce.h" and it seems another one is needed for "tensorflow/lite/micro/kernels/activation_utils.h" with fmin/fmax functions but I don't know how to fix it.

It looks like it's because C++ 11 is required. Does RISC-V not support that with RIOT at the moment? I know that is the case with ESP32

@benpicco
Copy link
Contributor

benpicco commented Apr 29, 2021

Can you try with

CXXEXFLAGS += -std=c++11

in tensorflow-lite/Makefile.include? The you could probably also drop the existing patch.

flatbuffers sets this too. (I wonder why it's not the default given that we already compile with -std=c11)

I think both RISC-V and esp32 toolchains should support C++11.

@sl1g18
Copy link
Author

sl1g18 commented Apr 29, 2021

Can you try with

CXXEXFLAGS += -std=c++11

in tensorflow-lite/Makefile.include? The you could probably also drop the existing patch.

flatbuffers sets this too. (I wonder why it's not the default given that we already compile with -std=c11)

I think both RISC-V and esp32 toolchains should support C++11.

Makefile.dep contains

USEMODULE += cpp11-compat
....
# C++ support on ESP32 in RIOT doesn't work with TensorFlow-Lite for the moment
FEATURES_BLACKLIST += arch_esp32

Which made me conclude that it doesn't work with ESP32. I will try your suggestion, but I'm getting a different error when compiling it locally.

Sergiu

@aabadie
Copy link
Contributor

aabadie commented Apr 29, 2021

I'm getting a different error when compiling it locally.

It might be your RISCV toolchain version. Use the riotbuild docker image and you'll get the same failure:

$ BOARD=hifive1b BUILD_IN_DOCKER=1 make -C tests/pkg_tensorflow-lite --no-print-directory 
Launching build container using image "riot/riotbuild:latest".
docker run --rm --tty --user $(id -u) -v '/usr/share/zoneinfo/Europe/Paris:/etc/localtime:ro' -v '/work/riot/RIOT:/data/riotbuild/riotbase:delegated' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -e 'BUILD_DIR=/data/riotbuild/riotbase/build' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles'      -e 'BOARD=hifive1b'  -w '/data/riotbuild/riotbase/tests/pkg_tensorflow-lite/' 'riot/riotbuild:latest' make  'EXTERNAL_MODULE_DIRS=/data/riotbuild/riotbase/tests/pkg_tensorflow-lite/mnist'   
Building application "tests_pkg_tensorflow-lite" for "hifive1b" with MCU "fe310".

"make" -C /data/riotbuild/riotbase/pkg/flatbuffers
"make" -C /data/riotbuild/riotbase/pkg/gemmlowp
"make" -C /data/riotbuild/riotbase/pkg/tensorflow-lite
"make" -C /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/c -f /data/riotbuild/riotbase/pkg/tensorflow-lite/Makefile.tensorflow-lite-c
"make" -C /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/core/api -f /data/riotbuild/riotbase/pkg/tensorflow-lite/Makefile.tensorflow-lite-core
"make" -C /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels -f /data/riotbuild/riotbase/pkg/tensorflow-lite/Makefile.tensorflow-lite-kernels
"make" -C /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal -f /data/riotbuild/riotbase/pkg/tensorflow-lite/Makefile.tensorflow-lite-kernels-internal
"make" -C /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/micro/kernels -f /data/riotbuild/riotbase/pkg/tensorflow-lite/Makefile.tensorflow-lite-micro-kernels
In file included from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/micro/kernels/reduce.cc:16:
/data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal/reference/reduce.h: In function 'bool tflite::reference_ops::QuantizedMeanOrSum(const T*, int32, float, const int*, int, T*, int32, float, const int*, int, const int*, int, bool, int*, int*, U*, bool)':
/data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal/reference/reduce.h:377:33: error: 'round' is not a member of 'std'; did you mean 'round'?
  377 |             static_cast<U>(std::round(temp_sum[idx] * scale + bias)) +
      |                                 ^~~~~
In file included from /opt/xpack-riscv-none-embed-gcc-10.1.0-1.1/riscv-none-embed/include/c++/10.1.0/cmath:45,
                 from /data/riotbuild/riotbase/build/pkg/gemmlowp/fixedpoint/fixedpoint.h:23,
                 from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal/common.h:24,
                 from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal/reference/reduce.h:19,
                 from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/micro/kernels/reduce.cc:16:
/opt/xpack-riscv-none-embed-gcc-10.1.0-1.1/riscv-none-embed/include/math.h:308:15: note: 'round' declared here
  308 | extern double round (double);
      |               ^~~~~
In file included from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/micro/kernels/reduce.cc:16:
/data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal/reference/reduce.h:387:27: error: 'round' is not a member of 'std'; did you mean 'round'?
  387 |             std::min(std::round(float_mean * scale + bias) + output_zero_point,
      |                           ^~~~~
In file included from /opt/xpack-riscv-none-embed-gcc-10.1.0-1.1/riscv-none-embed/include/c++/10.1.0/cmath:45,
                 from /data/riotbuild/riotbase/build/pkg/gemmlowp/fixedpoint/fixedpoint.h:23,
                 from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal/common.h:24,
                 from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/kernels/internal/reference/reduce.h:19,
                 from /data/riotbuild/riotbase/build/pkg/tensorflow-lite/tensorflow/lite/micro/kernels/reduce.cc:16:
/opt/xpack-riscv-none-embed-gcc-10.1.0-1.1/riscv-none-embed/include/math.h:308:15: note: 'round' declared here
  308 | extern double round (double);
      |               ^~~~~
/data/riotbuild/riotbase/Makefile.base:146: recipe for target '/data/riotbuild/riotbase/tests/pkg_tensorflow-lite/bin/hifive1b/tensorflow-lite-micro-kernels/reduce.o' failed
make[2]: *** [/data/riotbuild/riotbase/tests/pkg_tensorflow-lite/bin/hifive1b/tensorflow-lite-micro-kernels/reduce.o] Error 1
Makefile:20: recipe for target 'tensorflow-lite' failed
make[1]: *** [tensorflow-lite] Error 2
/data/riotbuild/riotbase/Makefile.include:709: recipe for target 'pkg-build-tensorflow-lite' failed
make: *** [pkg-build-tensorflow-lite] Error 2
make: *** [/work/riot/RIOT/makefiles/docker.inc.mk:314: ..in-docker-container] Error 2

The RISCV toolchain version in riotbuild Docker image is 10.1.0:

$ docker run --rm riot/riotbuild riscv-none-embed-gcc --version
riscv-none-embed-gcc (xPack GNU RISC-V Embedded GCC, 64-bit) 10.1.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@aabadie
Copy link
Contributor

aabadie commented Apr 29, 2021

Does RISC-V not support that with RIOT at the moment?

Normally it should but maybe the C++-11 support might vary depending on the toolchain.

Can you try with

CXXEXFLAGS += -std=c++11

This is already added in the flatbuffers Makefile.include file, so applied to the whole build, including tflite, since it depends on it.

# This module requires cpp11 support
CXXEXFLAGS += -std=c++11

@fjmolinas fjmolinas requested a review from aabadie May 3, 2021 08:50
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@benpicco benpicco requested a review from maribu September 22, 2021 19:59
@fjmolinas
Copy link
Contributor

@aabadie maybe you can take over this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants