Validate create graph parameters #3290

michalkulakowski · 2025-05-14T12:28:41Z

🛠 Summary

CVS-166727

🧪 Checklist

Unit tests added.
The documentation updated.
Change follows security best practices.
``

rasapala · 2025-05-27T08:18:45Z

src/config.cpp

@@ -92,6 +92,66 @@ bool Config::validate() {
            std::cerr << "Error: --task parameter not set." << std::endl;


Remove // TODO: CVS-1667

Not removed.

rasapala · 2025-05-27T08:22:01Z

src/config.cpp

+                std::cerr << "truncate: " << settings.truncate << " is not allowed. Supported values: true, false" << std::endl;
+                return false;
+            }
+        }


Add check for source_model and model_repository_path if they are set and add unit tests.

rasapala · 2025-05-27T08:22:57Z

src/config.cpp

+            }
+
+            if (serverSettings.hfSettings.sourceModel.rfind("OpenVINO/", 0) != 0) {
+                std::cerr << "For now only OpenVINO models are supported";


I think this should be generic check not only for text_generation. @dtrawins

rasapala · 2025-05-29T08:15:57Z

src/config.cpp

@@ -92,6 +92,67 @@ bool Config::validate() {
            std::cerr << "Error: --task parameter not set." << std::endl;
            return false;
        }
+#if (PYTHON_DISABLE == 1)


#if (PYTHON_DISABLE == 1) can be removed IMHO. We base optimum-cli functionality on external tools/python/environment not on internal ovms compilation flags. This check should be generic until I change it in optimum-cli enabling PR. @dtrawins

I think this is a valid check. The version without python shouldn't pull models outside of OV organization. Otherwise the pulled model will not work. It might change in the future but for how it looks reasonable.
@rasapala do you have in mind replacing it with a logic for checking if optimum-cli is present in runtime? It would be also fine when we have this detection.

rasapala · 2025-05-29T08:18:05Z

src/graph_export/graph_export.cpp

    // clang-format on
    std::string fullPath = FileSystem::joinPath({directoryPath, "graph.pbtxt"});
    return FileSystem::createFileOverwrite(fullPath, oss.str());
 }

+static Status validateSubconfigSchema(const std::string& subconfig, const std::string& type) {
+    rapidjson::Document subconfigJson;
+    rapidjson::ParseResult parseResult = subconfigJson.Parse(subconfig.c_str());


I am for parsing subconfig and mediapipe graph once it is created. @dtrawins want to remove it. Lets decide now.

@rasapala subconfig will not be used then new embeddings and rerank calculators are merged.

rasapala · 2025-05-29T08:19:04Z

src/graph_export/graph_export.cpp

+    ::mediapipe::CalculatorGraphConfig config;
+    bool success = ::google::protobuf::TextFormat::ParseFromString(oss.str(), &config);
+    if (!success) {
+        SPDLOG_ERROR("Created graph config couldn't be parsed - some parameters values are invalid.");


Created graph config couldn't be parsed - check used task parameters values.

I have mixed feelings if we should include this check in the graph export. It will be checked in the model initialization.
Do we know which parameters should cause such error? How is printed out? The log format might look strange for the CLI interface.

rasapala · 2025-05-29T08:19:20Z

src/graph_export/graph_export.cpp

+        return StatusCode::JSON_INVALID;
+    }
+    if (validateJsonAgainstSchema(subconfigJson, MEDIAPIPE_SUBCONFIG_SCHEMA.c_str()) != StatusCode::OK) {
+        SPDLOG_ERROR("Created {} subconfig file is not in valid configuration format", type);


Created graph config couldn't be parsed - check used task parameters values.

rasapala

@dtrawins please respond.

rasapala · 2025-05-29T08:38:16Z

Please change std::string "true":"false" flags to bool in cxxopts and settingsImpl.

dtrawins · 2025-05-22T12:42:55Z

src/config.cpp

+                return false;
+            }
+
+            std::vector allowedTargeDevices = {"CPU", "GPU", "NPU", "HETERO"};


Suggested change

std::vector allowedTargeDevices = {"CPU", "GPU", "NPU", "HETERO"};

std::vector allowedTargetDevices = {"CPU", "GPU", "NPU", "HETERO"};

dtrawins · 2025-05-22T12:45:32Z

src/config.cpp

+                return false;
+            }
+
+            std::vector allowedTargeDevices = {"CPU", "GPU", "NPU", "HETERO"};


that might be to restrictive

dtrawins · 2025-05-22T12:50:19Z

src/graph_export/graph_export.cpp

    // clang-format on
    std::string fullPath = FileSystem::joinPath({directoryPath, "graph.pbtxt"});
    return createFile(fullPath, oss.str());
 }

+static Status validateSubconfigSchema(const std::string& subconfig, const std::string& type) {


this is not needed

dtrawins · 2025-05-22T12:56:10Z

src/test/ovmsconfig_test.cpp

+        (char*)modelName.c_str(),
+        (char*)"--model_repository_path",
+        (char*)downloadPath.c_str(),
+        (char*)"--enable_prefix_caching",


we should be bool value

dtrawins · 2025-05-22T12:58:12Z

src/test/ovmsconfig_test.cpp

+        (char*)modelName.c_str(),
+        (char*)"--model_repository_path",
+        (char*)downloadPath.c_str(),
+        (char*)"--dynamic_split_fuse",


this could be bool flag

dtrawins · 2025-05-22T13:01:51Z

src/test/ovmsconfig_test.cpp

+        (char*)"--task",
+        (char*)"embeddings",
+        (char*)"--normalize",
+        (char*)"INVALID",


this could be bool option

dtrawins · 2025-05-29T08:48:58Z

src/config.cpp

@@ -92,6 +92,67 @@ bool Config::validate() {
            std::cerr << "Error: --task parameter not set." << std::endl;
            return false;
        }
+#if (PYTHON_DISABLE == 1)


I think this is a valid check. The version without python shouldn't pull models outside of OV organization. Otherwise the pulled model will not work. It might change in the future but for how it looks reasonable.
@rasapala do you have in mind replacing it with a logic for checking if optimum-cli is present in runtime? It would be also fine when we have this detection.

dtrawins · 2025-05-29T08:50:30Z

src/config.cpp

+                return false;
+            }
+
+            if (std::find(allowedBoolValues.begin(), allowedBoolValues.end(), settings.truncate) == allowedBoolValues.end()) {


truncate option will be effective only when convert_tokenizer is used? Maybe this parameter should be dropped for now?

dtrawins · 2025-05-29T08:54:10Z

src/graph_export/graph_export.cpp

+    ::mediapipe::CalculatorGraphConfig config;
+    bool success = ::google::protobuf::TextFormat::ParseFromString(oss.str(), &config);
+    if (!success) {
+        SPDLOG_ERROR("Created graph config couldn't be parsed - some parameters values are invalid.");


I have mixed feelings if we should include this check in the graph export. It will be checked in the model initialization.
Do we know which parameters should cause such error? How is printed out? The log format might look strange for the CLI interface.

dtrawins · 2025-05-29T08:55:19Z

src/graph_export/graph_export.cpp

    // clang-format on
    std::string fullPath = FileSystem::joinPath({directoryPath, "graph.pbtxt"});
    return FileSystem::createFileOverwrite(fullPath, oss.str());
 }

+static Status validateSubconfigSchema(const std::string& subconfig, const std::string& type) {
+    rapidjson::Document subconfigJson;
+    rapidjson::ParseResult parseResult = subconfigJson.Parse(subconfig.c_str());


@rasapala subconfig will not be used then new embeddings and rerank calculators are merged.

rasapala · 2025-05-29T13:33:10Z

src/test/ovmsconfig_test.cpp

+    };
+
+    int arg_count = 6;
+    EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), "or now OVMS version without python supports pulling OpenVINO models only");


Suggested change

EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), "or now OVMS version without python supports pulling OpenVINO models only");

EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), "Only OpenVINO models supported");

rasapala

1 change.

michalkulakowski requested a review from rasapala May 14, 2025 15:13

michalkulakowski force-pushed the mkulakow/create_graph_parameters_validation branch 3 times, most recently from 8c9921f to 1b2c917 Compare May 22, 2025 08:59

michalkulakowski requested a review from dtrawins May 22, 2025 12:38

rasapala requested changes May 27, 2025

View reviewed changes

michalkulakowski requested review from rasapala May 28, 2025 13:54

michalkulakowski force-pushed the mkulakow/create_graph_parameters_validation branch from 885923a to 42d22fb Compare May 29, 2025 08:04

rasapala reviewed May 29, 2025

View reviewed changes

rasapala requested changes May 29, 2025

View reviewed changes

dtrawins reviewed May 29, 2025

View reviewed changes

michalkulakowski requested review from rasapala and dtrawins May 29, 2025 09:58

michalkulakowski added 9 commits May 29, 2025 13:05

Validate create graph parameters

7277f7c

fix

bf1cbe6

fix

9119690

fix

5197ce8

fix

c66955c

fix

51923cd

Review fixes

29df3d8

fix

142ca0e

style

e63a6f2

michalkulakowski force-pushed the mkulakow/create_graph_parameters_validation branch from 0dc248d to e63a6f2 Compare May 29, 2025 11:07

dtrawins approved these changes May 29, 2025

View reviewed changes

fix

2ee56f0

rasapala reviewed May 29, 2025

View reviewed changes

rasapala requested changes May 29, 2025

View reviewed changes

fix

18ba9c6

rasapala approved these changes May 29, 2025

View reviewed changes

michalkulakowski merged commit d885cf3 into main May 29, 2025
14 checks passed

		@@ -92,6 +92,66 @@ bool Config::validate() {
		std::cerr << "Error: --task parameter not set." << std::endl;

	std::vector allowedTargeDevices = {"CPU", "GPU", "NPU", "HETERO"};
	std::vector allowedTargetDevices = {"CPU", "GPU", "NPU", "HETERO"};

	EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), "or now OVMS version without python supports pulling OpenVINO models only");
	EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), "Only OpenVINO models supported");

Validate create graph parameters #3290

Validate create graph parameters #3290

Uh oh!

Conversation

michalkulakowski commented May 14, 2025

🛠 Summary

🧪 Checklist

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!

rasapala May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rasapala left a comment

Choose a reason for hiding this comment

Uh oh!

rasapala commented May 29, 2025

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!

rasapala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rasapala May 29, 2025 •

edited

Loading