RSDK-13040 Split tflite_mlmodel code into a separate library #43

lvhg · 2026-01-02T21:04:26Z

https://viam.atlassian.net/browse/RSDK-13040

…nning

…ng correctlymake -j 4

lia-viam

various minor cleanup comments

lia-viam · 2026-01-12T22:03:47Z

build.zip

this may have been committed by accident

lia-viam · 2026-01-12T22:05:54Z

CMakeLists.txt

-  PRIVATE Threads::Threads
-  PRIVATE viam-cpp-sdk::viamsdk
-  PRIVATE tensorflow::tensorflowlite
+add_executable(main


I think it would make sense for the executable target to be called tflite_cpu_module or something like that, and the add_library target could just be called tflite_cpu (no _library) because the generated library file will have lib prefixed to it anyway (libtflite_cpu)

whichever you decide, however, you will probably need to commit changes to meta.json I think

lia-viam · 2026-01-12T22:07:07Z

src/tflite_cpu.hpp

+        // struct state_ final : public tflite::ErrorReporter;
+
+        // A visitor that can populate a TFLiteTensor given a MLModelService::tensor_view.
+        class write_to_tflite_tensor_visitor_ : public boost::static_visitor<TfLiteStatus>


I think this class should just appear in the header as a forward declaration class write_to_tflite_tensor_visitor_; and then have the full definition in the cpp file, similar to how we did with struct state_

I'd go so far as to suggest that this could likely move to the unnamed namespace in the .cpp file.

lia-viam · 2026-01-12T22:08:08Z

src/tflite_cpu.hpp

+#include <tensorflow/lite/interpreter_builder.h>
+#include <tensorflow/lite/kernels/register.h>
+
+#include <viam/sdk/common/instance.hpp>


I believe the following includes can/should be moved to main.cpp:

instance.hpp

registry.hpp

server.hpp

lia-viam · 2026-01-12T22:11:02Z

src/tflite_cpu.hpp

+#include <stdexcept>
+
+#include <tensorflow/lite/c/c_api.h>
+#include <tensorflow/lite/interpreter_builder.h>


the second two tensorflow includes can be moved to tflite_cpu.cpp I believe

I suspect all three can actually, because I think the only functions that reference the TF stuff in this header can actually be moved to free functions in the unnamed namespace in the .cpp file.

acmorrow

This is a good start.

I think the first and most important change is to get a .clang-format file in place and feed this back into the formatter. There is a lot of noise from formatting changes, and they are almost all formatting changes that we don't want. So it both makes it hard to review and isn't the end result we want. I'd prioritize that change.

I also think that several things that are currently member functions or class-scoped static functions / types can actually be pulled entirely out of the header / class and instead declared as free functions / types in the unnamed namespace in the .cpp file.

That in turn would allow removing the tflite headers from our header, which would mean the tflite-ness is fully encapsulated in the implementation: a nice outcome.

acmorrow · 2026-01-13T13:04:39Z

src/main.cpp

+namespace
+{
+
+    int serve(const std::string &socket_path)


It is a little weird. There is a bin/run-clang-format.sh script but no .clang-format file. I think what happened is that when this file was moved from its original home in the C++ SDK, the file got copied but the formatting rules didn't.

In any event, this definitely needs clang-format applied before merging.

src/main.cpp

acmorrow · 2026-01-13T13:09:21Z

src/main.cpp

@@ -0,0 +1,74 @@
+#include <shared_mutex>


Is shared_mutex used in this fiel?

acmorrow · 2026-01-13T13:10:30Z

src/tflite_cpu.cpp

+namespace mlmodel_tflite
+{


I'm not convinced this needs to be in a namespace at all. But, if you wanted to put it in one, I think I would put it in

namespace viam {

I was the one who suggested putting it in some namespace but I think you're right that since it's an application package, no namespace or namespace viam is fine

acmorrow · 2026-01-13T13:11:12Z

src/tflite_cpu.hpp

+#include <stdexcept>
+
+#include <tensorflow/lite/c/c_api.h>
+#include <tensorflow/lite/interpreter_builder.h>


I suspect all three can actually, because I think the only functions that reference the TF stuff in this header can actually be moved to free functions in the unnamed namespace in the .cpp file.

acmorrow · 2026-01-13T13:12:19Z

src/tflite_cpu.hpp

+#include <viam/sdk/common/proto_value.hpp>
+#include <viam/sdk/components/component.hpp>
+#include <viam/sdk/config/resource.hpp>
+#include <viam/sdk/module/service.hpp>
+#include <viam/sdk/registry/registry.hpp>
+#include <viam/sdk/resource/reconfigurable.hpp>
+#include <viam/sdk/resource/stoppable.hpp>
+#include <viam/sdk/rpc/server.hpp>
+#include <viam/sdk/services/mlmodel.hpp>


I expect this list can also be reduced. I'd start with just including mlmodel.hpp, stoppable.hpp, and reconfigurable.hpp, and only adding back what you really need beyond that.

acmorrow · 2026-01-13T13:12:53Z

src/tflite_cpu.hpp

+        // struct state_ final : public tflite::ErrorReporter;
+
+        // A visitor that can populate a TFLiteTensor given a MLModelService::tensor_view.
+        class write_to_tflite_tensor_visitor_ : public boost::static_visitor<TfLiteStatus>


I'd go so far as to suggest that this could likely move to the unnamed namespace in the .cpp file.

acmorrow · 2026-01-13T13:13:22Z

src/tflite_cpu.hpp

+        // All of the meaningful internal state of the service is held in
+        // a separate state object to help ensure clean replacement of our
+        // internals during reconfiguration.
+        // struct state_ final : public tflite::ErrorReporter;


acmorrow · 2026-01-13T13:15:12Z

src/tflite_cpu.hpp

+        MLModelService::tensor_views make_tensor_view_(const MLModelService::tensor_info &info,
+                                                       const TfLiteTensor *const tflite_tensor);
+
+        template <typename T>
+        MLModelService::tensor_views make_tensor_view_t_(const MLModelService::tensor_info &info,
+                                                         const TfLiteTensor *const tflite_tensor);


I think there is a decent possibility that these functions could be defined in an unnamed namespace in the .cpp file.

I'm a bit confused on how that would work since they're functions of MLModelServiceTFLite which is in a named namespace (mlmodel_tflite)

I'm a bit confused on how that would work since they're functions of MLModelServiceTFLite which is in a named namespace (mlmodel_tflite)

Because I don't think the actually need to be members of class MLModelServiceTFLite. I don't think they actually access any member state.

acmorrow · 2026-01-13T13:19:20Z

src/tflite_cpu.cpp


-    ~MLModelServiceTFLite() final {
+    MLModelServiceTFLite::~MLModelServiceTFLite()
+    {


The formatting changes are making it hard to see what really changed. Can we try to get a .clang-format file into this repo (probably just copy the one from the C++ SDK) and reformat?

acmorrow · 2026-01-13T18:32:09Z

@lvhg - I think it would be helpful if you could share what sort of testing is envisioned. Splitting this up into a header and source file is a necessary condition to being able to have tests that share the code, but it may not be sufficient, depending on what sort of tests you are trying to port over. Once this review lands, what you will be able to do is to instantiate a full tflite_cpu MLModelService instance, but is that really the intended unit of test? You would be more or less limited to making calls to create an instance with a configuration, invoke its metadata and infer endpoints, etc. Maybe that is enough? Maybe it isn't?

lvhg · 2026-01-13T20:54:00Z

@lvhg - I think it would be helpful if you could share what sort of testing is envisioned. Splitting this up into a header and source file is a necessary condition to being able to have tests that share the code, but it may not be sufficient, depending on what sort of tests you are trying to port over. Once this review lands, what you will be able to do is to instantiate a full tflite_cpu MLModelService instance, but is that really the intended unit of test? You would be more or less limited to making calls to create an instance with a configuration, invoke its metadata and infer endpoints, etc. Maybe that is enough? Maybe it isn't?

@acmorrow https://github.com/viam-modules/mlmodel-tflite/blob/0.0.5/tflitecpu/tflite_cpu_test.go This is the Go test file that we are planning to eventually convert into C++.

acmorrow

This is looking good. There are a decent number of comments here in this round, but they are all pretty minor and amount to:

Naming nits, questions, and suggestions
Requests to reduce scope/visibility or move things into the .cpp file and out of the class declaration in the header.

.github/workflows/test.yml

.clang-format

acmorrow · 2026-01-15T12:45:14Z

.gitignore

+build-conan/
+build/
+
+.DS_Store


.DS_Store probably belongs in your own ~/.gitignore, not the per project one.

Not sure what you mean by this, this is the only gitignore in the repository

You can have a personal gitignore file in your home directory.

acmorrow · 2026-01-15T12:48:12Z

CMakeLists.txt

+  src/main.cpp
 )

+target_link_libraries(tflite_cpu_module PRIVATE tflite_cpu)


I think you should do this the other way: leave the name of the executable unchanged, and find a different name for the libary. Otherwise, you need to change meta.json. Maybe call the library tflite_cpu_service.

Let's update this per my suggestion, since Lia seems to agree.

acmorrow · 2026-01-15T12:48:38Z

CMakeLists.txt

-  PRIVATE tensorflow::tensorflowlite
+  PUBLIC Threads::Threads
+  PUBLIC viam-cpp-sdk::viamsdk
+  PUBLIC tensorflow::tensorflowlite


I suspect that tensorflow::tensorflowlite is not actually a PUBLIC dependency of the library (or, at least, it won't be after you push the functions I called out down into the .cpp file). In that case, it can and should be PRIVATE. I agree that the viamsdk is PUBLIC.

I tried changing it to private, but it didn't work since some parameters referenced in hpp have TF types

Which, not-really-coincidentally, are the functions I'm encouraging you to get ouf of the header and into the .cpp file.

I expect this change will work now.

acmorrow · 2026-01-15T13:01:28Z

src/main.cpp

+}  // namespace
+
+int main(int argc, char* argv[]) {
+    const std::string usage = "usage: mlmodelservice_tflite /path/to/unix/socket";


Is this correct? I don't think mlmodelservice_tflite is the name of the binary anymore. In fact, the best way to get the name of the binary (or, at least, the name by which the binary was invoked) is out of argv[0], generally.

acmorrow · 2026-01-15T13:02:38Z

src/tflite_cpu.cpp

-        // automatically without needing to wait for anything more to
-        // drain.
-    }
+// namespace vsdk = ::viam::sdk;


Remove if not needed

src/tflite_cpu.cpp

acmorrow · 2026-01-15T13:08:08Z

src/tflite_cpu.cpp

+        tensor_data, tensor_size_t, std::move(shape));
 }
+
+}  // namespace mlmodel_tflite


File should end with a newline, please configure your editor to include it

acmorrow · 2026-01-15T13:08:40Z

.gitignore

+build-conan/
+build/
+
+.DS_Store


File should end with a newline, please configure your editor to automatically add it.

…work currently

…lServiceTFLite final

acmorrow

Only a few little things left now, this looks to me like it can be ready with one more round of review.

.github/workflows/test.yml

src/tflite_cpu.cpp

acmorrow · 2026-01-20T14:16:14Z

src/tflite_cpu.cpp

+
+// Converts from tflites type enumeration into the model service
+// type enumeration or throws if there is no such conversion.
+static MLModelService::tensor_info::data_types service_data_type_from_tflite_data_type_(


You don't need the static keyword here: that made sense when it was in a class, but you don't need it inside the unnamed namespace (which implies static).

There should not be a trailing _ on the name of this function because it is no longer a private class member.

acmorrow · 2026-01-20T14:18:25Z

.gitignore

+build-conan/
+build/
+
+.DS_Store


Again, I think .DS_Store belongs in your personal gitignore, not the project one.

acmorrow · 2026-01-20T14:18:59Z

CMakeLists.txt

-  PRIVATE tensorflow::tensorflowlite
+  PUBLIC Threads::Threads
+  PUBLIC viam-cpp-sdk::viamsdk
+  PUBLIC tensorflow::tensorflowlite


I expect this change will work now.

acmorrow · 2026-01-20T14:19:16Z

CMakeLists.txt

+  src/main.cpp
 )

+target_link_libraries(tflite_cpu_module PRIVATE tflite_cpu)


Let's update this per my suggestion, since Lia seems to agree.

acmorrow · 2026-01-20T14:20:28Z

src/main.cpp

+}  // namespace
+
+int main(int argc, char* argv[]) {
+    const std::string usage = "usage: mlmodelservice_tflite /path/to/unix/socket";


…ignature

acmorrow

I took a thorough pass through, and while I did find a handful of things, I really believe these are the final issues before this can merge, and none of them will take more than a few minutes to change.

acmorrow · 2026-01-20T22:04:20Z

src/main.cpp

+#include <viam/sdk/common/instance.hpp>
+#include <viam/sdk/module/service.hpp>
+#include <viam/sdk/registry/registry.hpp>
+// TODO import tflite_cpu library


Please remove this commented line before merging, but leave a blank line between the C++ SDK includes and the tflite_cpu.hpp one, to group them.

src/main.cpp

src/tflite_cpu.cpp

acmorrow · 2026-01-20T22:09:05Z

src/tflite_cpu.cpp

+namespace mlmodel_tflite {

+namespace {
+using namespace viam::sdk;


You should remove using namespace viam::sdk now that you have namespace vsdk = ::viam::sdk so you know you got everything.

CMakeLists.txt

acmorrow · 2026-01-20T22:12:08Z

CMakeLists.txt

  PRIVATE tensorflow::tensorflowlite
 )

-install(


Hmm. What happened to the install target? Was this intentional? If so, why?

acmorrow · 2026-01-20T22:17:20Z

src/main.cpp

+
+int main(int argc, char* argv[]) {
+    const std::string usage =
+        std::string("usage: ") + std::string(argv[0]) + std::string(" /path/to/unix/socket\n");


std::string has an operator+ overload that accepts const char*, so I don't think you need to wrap in std::string, and the type is definitely string so you don't need to name it on the LHS:

const auto usage = std::string("usage: ") + argv[0] + " /path/to/unix/socket";

You also don't need the terminating \n here if you have one on line 60, or vice versa. Probably better to retain the one on 60 and remove the one here.

acmorrow · 2026-01-20T22:18:59Z

src/tflite_cpu.cpp

+}
+
+// A visitor that can populate a TFLiteTensor given a MLModelService::tensor_view.
+class write_to_tflite_tensor_visitor_ : public boost::static_visitor<TfLiteStatus> {


This type is no longer private, so you can remove the trailing _.

This type also belongs in the unnamed namespace with tensor_views_from_tflite_tensor and tensor_views_from_tflite_tensor[,_t]

acmorrow · 2026-01-20T22:23:54Z

src/tflite_cpu.cpp

+    const std::string* name_;
+    TfLiteTensor* tflite_tensor_;


These fields were originally private to write_to_tflite_tensor_visitor_ (note the trailing _ on the names), but they have since become public. I think they should remain private:.

acmorrow

LGTM, with one small cleanup regarding usage of the vsdk:: namespace alias, and two places trailing newlines should be added/restored.

But, you don't need to post another round of review. Just fix those up before merging.

acmorrow · 2026-01-21T20:25:42Z

src/tflite_cpu.cpp

-    class write_to_tflite_tensor_visitor_;
+namespace {
+namespace vsdk = ::viam::sdk;
+using namespace vsdk;


You should entirely remove the line using namespace vsdk and everything should keep working, because all C++ SDK usages in the .cpp file should now be vsdk:: qualified, as they originally were. If there are any missing, just add vsdk:: where needed.

acmorrow · 2026-01-21T20:27:28Z

src/tflite_cpu.cpp

 }
+
+}  // namespace mlmodel_tflite
+}  // namespace viam


Please restore the trailing newlines and configure your editor to add them.

acmorrow · 2026-01-21T20:27:37Z

src/tflite_cpu.hpp

+};
+
+}  // namespace mlmodel_tflite
+}


Trailing newline here too

Lillian Hwang-Geddes added 25 commits December 17, 2025 13:43

start writing the tests and header

7578f04

attempt to build cmake

9a7888f

update cmakelists and conanfile

c02a3b9

add main file

bfa82d5

update header file

d1ee749

planning for tests

d7f9c75

move cmakefiles and cmakecache

c503977

add to gitignore file

df53bad

remove tests

0b32815

add to cmakelists, remove build documents from git

489b41c

fix library linking, fix imports

3664c41

fix header file problems

ddca157

move to header file

ed09b6f

move class definition to header file

55c554f

fix syntax errors

f2379be

move some small methods to cpp

a61a9ad

move most methods to cpp

5e99066

fix some of the errors in hpp and cpp files, but still make is not ru…

42e14ce

…nning

moved methods from write_to_tflite_tensor_visitor to cpp

e1773a1

add back metadata function that somehow got removed

6d17ea6

remove alias

4c7f65a

add constructor to hpp file and remove alias from main. make is worki…

34c5374

…ng correctlymake -j 4

move write_to_tflite_tensor_visitor_ definition to cpp

aa27c58

remove comments

cf0e95a

remove macos 13 as it has been retired

df89d23

lvhg requested review from acmorrow, bhaney and lia-viam January 12, 2026 19:57

lia-viam requested changes Jan 12, 2026

View reviewed changes

acmorrow requested changes Jan 13, 2026

View reviewed changes

Lillian Hwang-Geddes added 2 commits January 13, 2026 16:10

fix formatting with clang-format

72fea80

change executable/library name and remove unnecessary imports

be68410

acmorrow requested changes Jan 15, 2026

View reviewed changes

Lillian Hwang-Geddes added 7 commits January 15, 2026 13:56

move write_to_tflite_tensor_visitor class to cpp

a7a04c2

fix write to tflite class errors

7efc8d9

formatting, removing comments, move service_name to cpp

8f5a0ca

move make_tensor_view methods to cpp file and rename them - does not …

28668ee

…work currently

fix errors related to tensor_views_from_tflite_tensor

3ce48ee

move service_data_type_from_tflite_data_type to cpp file, make MLMode…

2429753

…lServiceTFLite final

remove unnecessary imports, namespace changes

29a8770

acmorrow requested changes Jan 20, 2026

View reviewed changes

Lillian Hwang-Geddes added 2 commits January 20, 2026 15:33

change executable/library name, fix binary name, change method name/s…

bd576dc

…ignature

put namespace alias back

49e4f9d

acmorrow requested changes Jan 20, 2026

View reviewed changes

namespace/variable name changes

14756f0

acmorrow approved these changes Jan 21, 2026

View reviewed changes

RSDK-13040 Split tflite_mlmodel code into a separate library #43

Are you sure you want to change the base?

RSDK-13040 Split tflite_mlmodel code into a separate library #43

Uh oh!

Conversation

lvhg commented Jan 2, 2026

Uh oh!

lia-viam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acmorrow commented Jan 13, 2026

Uh oh!

lvhg commented Jan 13, 2026

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment