Adress & remove TODO's (#61)

Remove TODO from Model API adapter. We should change API there but this is in different repo. Created tickets: CVS-128781 CVS-128782 CVS-128783 CVS-128784 CVS-128785 * Add missing test files * Add calculators tests to CI JIRA:CVS-127914
openvinotoolkit · Jan 8, 2024 · b0a0e72 · b0a0e72
1 parent 00612f2
commit b0a0e72
Show file tree

Hide file tree

Showing 11 changed files with 115 additions and 372 deletions.
diff --git a/Makefile b/Makefile
@@ -40,6 +40,7 @@ run_hello_ovms:
 MEDIAPIPE_UNSTABLE_TESTS_REGEX="MuxInputStreamHandlerTest.RemovesUnusedDataStreamPackets"
 run_unit_tests:
 	docker run -e http_proxy=$(HTTP_PROXY) -e https_proxy=$(HTTPS_PROXY) $(OVMS_MEDIA_DOCKER_IMAGE):$(OVMS_MEDIA_IMAGE_TAG) bazel test --define=MEDIAPIPE_DISABLE_GPU=1 --test_output=streamed --test_filter="-${MEDIAPIPE_UNSTABLE_TESTS_REGEX}" //mediapipe/framework/...
+	docker run -e http_proxy=$(HTTP_PROXY) -e https_proxy=$(HTTPS_PROXY) $(OVMS_MEDIA_DOCKER_IMAGE):$(OVMS_MEDIA_IMAGE_TAG) bazel test --define=MEDIAPIPE_DISABLE_GPU=1 --test_output=streamed //mediapipe/calculators/ovms:all
 
 run_hello_world:
 	docker run $(OVMS_MEDIA_DOCKER_IMAGE):$(OVMS_MEDIA_IMAGE_TAG) bazel-bin/mediapipe/examples/desktop/hello_world/hello_world

diff --git a/mediapipe/calculators/ovms/BUILD b/mediapipe/calculators/ovms/BUILD
@@ -47,7 +47,7 @@ cc_library(
         "modelapiovmsadapter.hpp"
     ],
     deps = [
-        "//mediapipe/framework/port:logging", # TODO remove logs but need better error handling/reporting in Model API
+        "//mediapipe/framework/port:logging",
         "@ovms//src:ovms_header",
         "@model_api//:model_api",
         "@linux_openvino//:openvino",
@@ -145,8 +145,8 @@ cc_test(
         "test_data/config.json",
         "test_data/dummy/1/dummy.bin",
         "test_data/dummy/1/dummy.xml",
-        "test_data/add_two_inputs_model/1/dummy.bin",
-        "test_data/add_two_inputs_model/1/dummy.xml",
+        "test_data/add_two_inputs_model/1/add.bin",
+        "test_data/add_two_inputs_model/1/add.xml",
     ],
     copts = ["-Iexternal/ovms/src","-Isrc"],
 )
diff --git a/mediapipe/calculators/ovms/modelapiovmsadapter.cc b/mediapipe/calculators/ovms/modelapiovmsadapter.cc
@@ -67,14 +67,11 @@ using std::endl;
 #pragma GCC diagnostic ignored "-Wunused-function"
 using InferenceOutput = std::map<std::string, ov::Tensor>;
 using InferenceInput = std::map<std::string, ov::Tensor>;
-// TODO
-// * why std::map
-// * no ret code from infer()
-// * no ret code from load()
+
 namespace ovms {
 static OVMS_DataType OVPrecision2CAPI(ov::element::Type_t datatype);
 static ov::element::Type_t CAPI2OVPrecision(OVMS_DataType datatype);
-static ov::Tensor makeOvTensorO(OVMS_DataType datatype, const int64_t* shape, size_t dimCount, const void* voutputData, size_t bytesize);
+static ov::Tensor makeOvTensor(OVMS_DataType datatype, const int64_t* shape, size_t dimCount, const void* voutputData, size_t bytesize);
 
 OVMSInferenceAdapter::OVMSInferenceAdapter(const std::string& servableName, uint32_t servableVersion, OVMS_Server* cserver) :
     servableName(servableName),
@@ -103,31 +100,22 @@ InferenceOutput OVMSInferenceAdapter::infer(const InferenceInput& input) {
     // PREPARE EACH INPUT
     // extract single tensor
     for (const auto& [name, input_tensor] : input) {
-        // TODO validate existence of tag key in map
-        // or handle inference when there is no need for mapping
         const char* realInputName = name.c_str();
-#if 0
-        const float* input_tensor_access = reinterpret_cast<float*>(input_tensor.data());
-        std::stringstream ss;
-        ss << " Adapter received tensor: [ ";
-        for (int x = 0; x < 10; ++x) {
-            ss << input_tensor_access[x] << " ";
-        }
-        ss << " ]";
-        LOG(INFO) << ss.str();
-#endif
         const auto& ovinputShape = input_tensor.get_shape();
-        std::vector<int64_t> inputShape{ovinputShape.begin(), ovinputShape.end()};  // TODO error handling shape conversion
+        if (std::any_of(ovinputShape.begin(), ovinputShape.end(), [](size_t dim) {
+                return dim > std::numeric_limits<int64_t>::max();})) {
+            throw std::runtime_error("Cannot use C-API with dimension size greater than int64_t max value");
+        }
+        std::vector<int64_t> inputShape{ovinputShape.begin(), ovinputShape.end()};
         OVMS_DataType inputDataType = OVPrecision2CAPI(input_tensor.get_element_type());
-        ASSERT_CAPI_STATUS_NULL(OVMS_InferenceRequestAddInput(request, realInputName, inputDataType, inputShape.data(), inputShape.size()));  // TODO retcode
+        ASSERT_CAPI_STATUS_NULL(OVMS_InferenceRequestAddInput(request, realInputName, inputDataType, inputShape.data(), inputShape.size()));
         const uint32_t NOT_USED_NUM = 0;
-        // TODO handle hardcoded buffertype, notUsedNum additional options? side packets?
         ASSERT_CAPI_STATUS_NULL(OVMS_InferenceRequestInputSetData(request,
             realInputName,
             reinterpret_cast<void*>(input_tensor.data()),
             input_tensor.get_byte_size(),
             OVMS_BUFFERTYPE_CPU,
-            NOT_USED_NUM));  // TODO retcode
+            NOT_USED_NUM));
     }
     //////////////////
     //  INFERENCE
@@ -147,13 +135,10 @@ InferenceOutput OVMSInferenceAdapter::infer(const InferenceInput& input) {
         return output;
     }
     CREATE_GUARD(responseGuard, OVMS_InferenceResponse, response);
-    // verify GetOutputCount
     uint32_t outputCount = 42;
     ASSERT_CAPI_STATUS_NULL(OVMS_InferenceResponseOutputCount(response, &outputCount));
     uint32_t parameterCount = 42;
     ASSERT_CAPI_STATUS_NULL(OVMS_InferenceResponseParameterCount(response, &parameterCount));
-    // TODO handle output filtering. Graph definition could suggest
-    // that we are not interested in all outputs from OVMS Inference
     const void* voutputData;
     size_t bytesize = 42;
     OVMS_DataType datatype = (OVMS_DataType)199;
@@ -164,18 +149,19 @@ InferenceOutput OVMSInferenceAdapter::infer(const InferenceInput& input) {
     const char* outputName{nullptr};
     for (size_t i = 0; i < outputCount; ++i) {
         ASSERT_CAPI_STATUS_NULL(OVMS_InferenceResponseOutput(response, i, &outputName, &datatype, &shape, &dimCount, &voutputData, &bytesize, &bufferType, &deviceId));
-        output[outputName] = makeOvTensorO(datatype, shape, dimCount, voutputData, bytesize);  // TODO optimize FIXME
+        output[outputName] = makeOvTensor(datatype, shape, dimCount, voutputData, bytesize);
     }
     return output;
 }
+
 void OVMSInferenceAdapter::loadModel(const std::shared_ptr<const ov::Model>& model, ov::Core& core,
     const std::string& device, const ov::AnyMap& compilationConfig) {
     // no need to load but we need to extract metadata
     OVMS_ServableMetadata* servableMetadata = nullptr;
     ASSERT_CAPI_STATUS_NULL(OVMS_GetServableMetadata(cserver, servableName.c_str(), servableVersion, &servableMetadata));
+    CREATE_GUARD(metadataGuard, OVMS_ServableMetadata, servableMetadata);
     uint32_t inputCount = 0;
     uint32_t outputCount = 0;
-    // TODO ensure Metadata object removal in all paths
     ASSERT_CAPI_STATUS_NULL(OVMS_ServableMetadataInputCount(servableMetadata, &inputCount));
     ASSERT_CAPI_STATUS_NULL(OVMS_ServableMetadataOutputCount(servableMetadata, &outputCount));
 
@@ -190,7 +176,6 @@ void OVMSInferenceAdapter::loadModel(const std::shared_ptr<const ov::Model>& mod
         inputNames.emplace_back(tensorName);
         shape_min_max_t inputMinMax;
         for (size_t i = 0; i < dimCount; ++i) {
-            // TODO test adapter dynamic shapes
             inputMinMax.first.emplace_back(shapeMin[i]);
             inputMinMax.second.emplace_back(shapeMax[i]);
         }
@@ -203,7 +188,6 @@ void OVMSInferenceAdapter::loadModel(const std::shared_ptr<const ov::Model>& mod
     const ov::AnyMap* servableMetadataRtInfo;
     ASSERT_CAPI_STATUS_NULL(OVMS_ServableMetadataInfo(servableMetadata, reinterpret_cast<const void**>(&servableMetadataRtInfo)));
     this->modelConfig = *servableMetadataRtInfo;
-    OVMS_ServableMetadataDelete(servableMetadata);
 }
 
 ov::PartialShape OVMSInferenceAdapter::getInputShape(const std::string& inputName) const {
@@ -294,7 +278,7 @@ static ov::element::Type_t CAPI2OVPrecision(OVMS_DataType datatype) {
     return it->second;
 }
 
-static ov::Tensor makeOvTensorO(OVMS_DataType datatype, const int64_t* shape, size_t dimCount, const void* voutputData, size_t bytesize) {
+static ov::Tensor makeOvTensor(OVMS_DataType datatype, const int64_t* shape, size_t dimCount, const void* voutputData, size_t bytesize) {
     ov::Shape ovShape;
     for (size_t i = 0; i < dimCount; ++i) {
         ovShape.push_back(shape[i]);

diff --git a/mediapipe/calculators/ovms/modelapiovmsadapter.hpp b/mediapipe/calculators/ovms/modelapiovmsadapter.hpp
@@ -23,7 +23,7 @@
 #include <utility>
 #include <vector>
 
-#include <adapters/inference_adapter.h>  // TODO fix path  model_api/model_api/cpp/adapters/include/adapters/inference_adapter.h
+#include <adapters/inference_adapter.h>  // model_api/model_api/cpp/adapters/include/adapters/inference_adapter.h
 #include <openvino/openvino.hpp>
 
 // here we need to decide if we have several calculators (1 for OVMS repository, 1-N inside mediapipe)
@@ -37,8 +37,6 @@ namespace ovms {
 using InferenceOutput = std::map<std::string, ov::Tensor>;
 using InferenceInput = std::map<std::string, ov::Tensor>;
 
-// TODO
-// * why std::map
 using shape_border_t = std::vector<int64_t>;
 using shape_min_max_t = std::pair<shape_border_t, shape_border_t>;
 using shapes_min_max_t = std::unordered_map<std::string, shape_min_max_t>;

diff --git a/mediapipe/calculators/ovms/openvinoinferencecalculator.cc b/mediapipe/calculators/ovms/openvinoinferencecalculator.cc
@@ -20,7 +20,7 @@
 #include <sstream>
 #include <unordered_map>
 
-#include <adapters/inference_adapter.h>  // TODO fix path  model_api/model_api/cpp/adapters/include/adapters/inference_adapter.h
+#include <adapters/inference_adapter.h>  // model_api/model_api/cpp/adapters/include/adapters/inference_adapter.h
 #include <openvino/core/shape.hpp>
 #include <openvino/openvino.hpp>
 
@@ -317,7 +317,7 @@ static ov::Tensor convertTFLiteTensor2OVTensor(const TfLiteTensor& t) {
     ov::Shape shape;
     // for some reason TfLite tensor does not have bs dim
     shape.emplace_back(1);
-    // TODO: Support scalars and no data tensors with 0-dim
+    // No support for scalars and no data tensors with 0-dim
     for (int i = 0; i < t.dims->size; ++i) {
         shape.emplace_back(t.dims->data[i]);
     }
@@ -570,7 +570,6 @@ class OpenVINOInferenceCalculator : public CalculatorBase {
                 cc->Outputs().Tag(tag).Add(
                     tensors.release(),
                     cc->InputTimestamp());
-                //break; // TODO FIXME order of outputs
                 // no need to break since we only have one tag
                 // create concatenator calc
             } else if (startsWith(tag, MPTENSORS_TAG)) {
@@ -599,11 +598,9 @@ class OpenVINOInferenceCalculator : public CalculatorBase {
                 cc->Outputs().Tag(tag).Add(
                     tensors.release(),
                     cc->InputTimestamp());
-                //break; // TODO FIXME order of outputs
                 // no need to break since we only have one tag
                 // create concatenator calc
             } else if (startsWith(tag, TFLITE_TENSORS_TAG)) {
-                // TODO FIXME use output_order_list
                 LOG(INFO) << "OVMS calculator will process vector<TfLiteTensor>";
                 auto outputStreamTensors = std::vector<TfLiteTensor>();
                 if (!this->initialized) {
@@ -619,7 +616,7 @@ class OpenVINOInferenceCalculator : public CalculatorBase {
                         }
                         interpreter_->SetTensorParametersReadWrite(
                                         tensorId,
-                                        kTfLiteFloat32, // TODO datatype
+                                        kTfLiteFloat32,
                                         name.c_str(),
                                         tfliteshape,
                                         TfLiteQuantization());

diff --git a/mediapipe/calculators/ovms/openvinomodelserversessioncalculator.proto b/mediapipe/calculators/ovms/openvinomodelserversessioncalculator.proto
@@ -29,5 +29,7 @@ message OpenVINOModelServerSessionCalculatorOptions {
     optional string servable_version = 2;
     // service_url: "13.21.212.171:9718"
     optional string service_url = 3;
+    // config_path: "/models/config.json"
+    // when this field is used ensure that each calculator is using the same file
     optional string server_config = 4;
 }
diff --git a/mediapipe/calculators/ovms/openvinomodelserversessioncalculator_test.cc b/mediapipe/calculators/ovms/openvinomodelserversessioncalculator_test.cc
@@ -23,7 +23,7 @@
 
 #include "mediapipe/framework/port/gtest.h"
 
-#include <adapters/inference_adapter.h>  // TODO fix path  model_api/model_api/cpp/adapters/include/adapters/inference_adapter.h
+#include <adapters/inference_adapter.h>  // model_api/model_api/cpp/adapters/include/adapters/inference_adapter.h
 #include <openvino/core/shape.hpp>
 #include <openvino/openvino.hpp>