add_to_config and remove_from_config user interface improvements (#3602)

* changed in logging config path assuming configPath field containing path to the file

* changed a way to parse config path

* changed error message to show examples of commands

* clang format change

* changed --help and error infos regarding add_to_config and remove_from_config

* changed of config management commands examples, removed [OPTION...] suffix

* fixed OvmsConfigDeathTest tests

* fixed ConfigCreationTest tests, added to updating config check if configPath is empty

* corrected info about config options in --help

---------

Co-authored-by: Trawinski, Dariusz <dariusz.trawinski@intel.com>

Author: przepeck

src/BUILD

@@ -305,6 +305,7 @@ ovms_cc_library(
         "libovms_server_settings",
         "libovms_version",
         "libovmsfilesystem",
+        "libovmslocalfilesystem",
         "//src/graph_export:graph_cli_parser",
         "//src/graph_export:rerank_graph_cli_parser",
         "//src/graph_export:embeddings_graph_cli_parser",

src/cli_parser.cpp

@@ -28,16 +28,30 @@
 #include "graph_export/image_generation_graph_cli_parser.hpp"
 #include "ovms_exit_codes.hpp"
 #include "filesystem.hpp"
+#include "localfilesystem.hpp"
 #include "stringutils.hpp"
 #include "version.hpp"
 
 namespace ovms {
 
 constexpr const char* CONFIG_MANAGEMENT_HELP_GROUP{"config management"};
 
+std::string getConfigPath(const std::string& configPath) {
+    bool isDir = false;
+    auto status = LocalFileSystem::isDir(configPath, &isDir);
+    if (!status.ok()) {
+        throw std::logic_error("Invalid path for the config: " + configPath);
+    }
+    if (isDir) {
+        return FileSystem::joinPath({configPath, "config.json"});
+    }
+    return configPath;
+}
+
 void CLIParser::parse(int argc, char** argv) {
     try {
         options = std::make_unique<cxxopts::Options>(argv[0], "OpenVINO Model Server");
+        auto configOptions = std::make_unique<cxxopts::Options>("ovms --model_name <MODEL_NAME> --add_to_config <CONFIG_PATH> --model_repository_path <MODEL_REPO_PATH> \n  ovms --model_path <MODEL_PATH> --model_name <MODEL_NAME> --add_to_config <CONFIG_PATH> \n  ovms --remove_from_config <CONFIG_PATH> --model_name <MODEL_NAME>", "config management commands:");
         // Adding this option to parse unrecognised options in another parser
         options->allow_unrecognised_options();
 
@@ -254,6 +268,33 @@ void CLIParser::parse(int argc, char** argv) {
                 "Determines how many sequences can be processed concurrently by one model instance. When that value is reached, attempt to start a new sequence will result in error.",
                 cxxopts::value<uint32_t>(),
                 "MAX_SEQUENCE_NUMBER");
+        configOptions->custom_help("");
+        configOptions->add_options(CONFIG_MANAGEMENT_HELP_GROUP)
+            ("list_models",
+                "Directive to show available servables in models repository",
+                cxxopts::value<bool>()->default_value("false"),
+                "LIST_MODELS")
+            ("add_to_config",
+                "Either path to directory containing config.json file for OVMS, or path to ovms configuration file, to add specific model to. This parameter should be executed with --model_name and either with --model_path or --model_repository_path.",
+                cxxopts::value<std::string>(),
+                "ADD_TO_CONFIG")
+            ("remove_from_config",
+                "Either path to directory containing config.json file for OVMS, or path to ovms configuration file, to remove specific model from. This parameter should be executed with --model_name to specify which model we want to remove.",
+                cxxopts::value<std::string>(),
+                "REMOVE_FROM_CONFIG")
+            ("model_repository_path",
+                "Absolute or relative path from the config directory to the model repository",
+                cxxopts::value<std::string>(),
+                "MODEL_REPOSITORY_PATH")
+            ("model_path",
+                "Absolute or relative path from the config directory to the model. By default is a combination of the model_repository_path and model_name.",
+                cxxopts::value<std::string>(),
+                "MODEL_PATH")
+            ("model_name",
+                "Name of the model",
+                cxxopts::value<std::string>(),
+                "MODEL_NAME");
+
         result = std::make_unique<cxxopts::ParseResult>(options->parse(argc, argv));
 
         // HF pull mode or pull and start mode
@@ -358,7 +399,8 @@ void CLIParser::parse(int argc, char** argv) {
         }
 
         if (result->count("help") || result->arguments().size() == 0) {
-            std::cout << options->help({"", "multi model", "single model", "pull hf model", CONFIG_MANAGEMENT_HELP_GROUP}) << std::endl;
+            std::cout << options->help({"", "multi model", "single model", "pull hf model"}) << std::endl;
+            std::cout << configOptions->help({CONFIG_MANAGEMENT_HELP_GROUP}) << std::endl;
             GraphCLIParser parser1;
             RerankGraphCLIParser parser2;
             EmbeddingsGraphCLIParser parser3;
@@ -461,7 +503,7 @@ void CLIParser::prepareModel(ModelsSettingsImpl& modelsSettings, HFSettingsImpl&
     }
     if (result->count("model_path")) {
         modelsSettings.modelPath = result->operator[]("model_path").as<std::string>();
-        modelsSettings.userSetSingleModelArguments.push_back("model_name");
+        modelsSettings.userSetSingleModelArguments.push_back("model_path");
     }
     if (result->count("max_sequence_number")) {
         modelsSettings.maxSequenceNumber = result->operator[]("max_sequence_number").as<uint32_t>();
@@ -637,9 +679,9 @@ void CLIParser::prepareConfigExport(ModelsSettingsImpl& modelsSettings) {
         modelsSettings.modelPath = FileSystem::joinPath({result->operator[]("model_repository_path").as<std::string>(), modelsSettings.modelName});
     }
     if (result->count("add_to_config")) {
-        modelsSettings.configPath = result->operator[]("add_to_config").as<std::string>();
+        modelsSettings.configPath = ovms::getConfigPath(result->operator[]("add_to_config").as<std::string>());
     } else if (result->count("remove_from_config")) {
-        modelsSettings.configPath = result->operator[]("remove_from_config").as<std::string>();
+        modelsSettings.configPath = ovms::getConfigPath(result->operator[]("remove_from_config").as<std::string>());
     }
 }
 

src/config.cpp

@@ -230,15 +230,22 @@ bool Config::validate() {
         }
     } else {
         if (configPath().empty()) {
-            std::cerr << "Set config_path with add_to_config, remove_from_config" << std::endl;
+            std::cerr << "Set config_path with add_to_config/remove_from_config" << std::endl;
             return false;
         }
         if (modelName().empty()) {
-            std::cerr << "Set model_name with add_to_config, remove_from_config" << std::endl;
+            std::cerr << "Set model_name with add_to_config/remove_from_config" << std::endl
+                      << "Usage: " << std::endl
+                      << "  ovms --model_name <model_name> --model_repository_path <repo_path> --add_to_config <config_path>" << std::endl
+                      << "  ovms --model_name <model_name> --model_path <model_path> --add_to_config <config_path>" << std::endl
+                      << "  ovms --model_name <model_name> --remove_from_config <config_path>" << std::endl;
             return false;
         }
         if (modelPath().empty() && this->serverSettings.exportConfigType == ENABLE_MODEL) {
-            std::cerr << "Set model_path or model_repository_path and model_name with add_to_config, remove_from_config" << std::endl;
+            std::cerr << "Set model_name either with model_path or model_repository_path with add_to_config" << std::endl
+                      << "Usage: " << std::endl
+                      << "  ovms --model_name <model_name> --model_repository_path <repo_path> --add_to_config <config_path>" << std::endl
+                      << "  ovms --model_name <model_name> --model_path <model_path> --add_to_config <config_path>" << std::endl;
             return false;
         }
 

src/config_export_module/config_export.cpp

@@ -190,28 +190,11 @@ Status DisableModel(const std::string& configFilePath, const ModelsSettingsImpl&
     }
 }
 
-static Status validateAndPrepareConfigFilePath(std::string& configDirOrFilePath) {
-    if (configDirOrFilePath.empty()) {
-        SPDLOG_ERROR("Config path empty: {}", configDirOrFilePath);
-        return StatusCode::PATH_INVALID;
-    }
-    // check if the config path is a directory and if it is, append config.json
-    bool isDir = false;
-    auto status = LocalFileSystem::isDir(configDirOrFilePath, &isDir);
-    if (!status.ok()) {
-        return status;
-    }
-    if (isDir) {
-        configDirOrFilePath = FileSystem::joinPath({configDirOrFilePath, "config.json"});
-    }
-    return StatusCode::OK;
-}
-
 Status updateConfig(const ModelsSettingsImpl& modelSettings, const ConfigExportType& exportType) {
     std::string configFilePath = modelSettings.configPath;
-    auto status = validateAndPrepareConfigFilePath(configFilePath);
-    if (!status.ok()) {
-        return status;
+    if (configFilePath.empty()) {
+        SPDLOG_ERROR("Config path is empty.");
+        return StatusCode::PATH_INVALID;
     }
     if (exportType == ENABLE_MODEL) {
         return EnableModel(configFilePath, modelSettings);

src/servables_config_manager_module/servablesconfigmanagermodule.cpp

@@ -47,10 +47,9 @@ Status ServablesConfigManagerModule::start(const ovms::Config& config) {
     } else {
         auto status = updateConfig(config.getModelSettings(), config.getServerSettings().exportConfigType);
         if (status.ok()) {
-            std::cout << "Config updated: " << config.getModelSettings().configPath << std::string(1, std::filesystem::path::preferred_separator) << "config.json" << std::endl;
+            std::cout << "Config updated: " << config.getModelSettings().configPath << std::endl;
         } else {
-            std::cout << config.getModelSettings().configPath << std::string(1, std::filesystem::path::preferred_separator) << "config.json"
-                      << " error on config update : " << status.string() << std::endl;
+            std::cout << config.getModelSettings().configPath << " error on config update : " << status.string() << std::endl;
         }
         return status;
     }