Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves OVMS CLI flows around local generative models by introducing a dedicated --configure mode for generating/updating graph.pbtxt, and by expanding --add_to_config to support adding classic model parameters into config.json. It also refreshes --help output and updates tests/docs accordingly.
Changes:
- Added
--configuremode (no server start) to create/updategraph.pbtxtfor a local model based on--taskoptions. - Extended config-management (
--add_to_config) to accept and persist additional model parameters, switching JSON emission to RapidJSON. - Updated task-specific CLI help/validation and added/updated unit tests and documentation.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/ovmsconfig_test.cpp | Removes obsolete death test that assumed extra params are forbidden with --add_to_config. |
| src/test/config_export_test.cpp | Updates expected exported JSON formatting. |
| src/test/config_export_full_test.cpp | Adds end-to-end tests for --configure and --add_to_config --batch_size. |
| src/server.cpp | Adds CONFIGURE_MODE runtime path to generate graph.pbtxt and exit. |
| src/graph_export/t2s_graph_cli_parser.cpp | Updates help text; adds --vocoder; allows CONFIGURE_MODE in parser flow. |
| src/graph_export/s2t_graph_cli_parser.cpp | Updates help text; allows CONFIGURE_MODE in parser flow. |
| src/graph_export/rerank_graph_cli_parser.cpp | Updates help text; allows CONFIGURE_MODE in parser flow. |
| src/graph_export/image_generation_graph_cli_parser.cpp | Updates help text; allows CONFIGURE_MODE in parser flow. |
| src/graph_export/graph_cli_parser.cpp | Updates help text; allows CONFIGURE_MODE in parser flow. |
| src/graph_export/embeddings_graph_cli_parser.cpp | Updates help text; allows CONFIGURE_MODE in parser flow. |
| src/config.hpp | Updates config-management argument validation signature to include export type. |
| src/config.cpp | Expands allowed CLI args for config add/remove and updates validation for CONFIGURE_MODE. |
| src/config_export_module/config_export.cpp | Emits config JSON via RapidJSON and supports optional model fields on export. |
| src/cli_parser.hpp | Declares configure-mode detection helper. |
| src/cli_parser.cpp | Adds --configure, adjusts help printing, and relaxes preprocessing/layout checks for add-to-config. |
| src/capi_frontend/server_settings.hpp | Adds CONFIGURE_MODE enum value. |
| docs/parameters.md | Documents configure mode and expanded --add_to_config parameter support. |
Comment on lines
+164
to
+168
| if (exportType == ENABLE_MODEL) { | ||
| std::cerr << "Adding models to the configuration file does not support parameters: " << arguments << std::endl; | ||
| } else { | ||
| std::cerr << "Removing models from the configuration file allows passing only model_name parameter. Invalid parameters passed: " << arguments << std::endl; | ||
| } |
Comment on lines
125
to
+131
| SPDLOG_DEBUG("model_repository_path: {}", config.getServerSettings().hfSettings.downloadPath); | ||
| return; | ||
| } | ||
| if (config.getServerSettings().serverMode == CONFIGURE_MODE) { | ||
| SPDLOG_DEBUG("model_path: {}", config.modelPath()); | ||
| return; | ||
| } |
Comment on lines
+35
to
+68
| static void addJsonOrStringMember(rapidjson::Value& obj, const char* key, const std::string& value, rapidjson::Document::AllocatorType& alloc) { | ||
| rapidjson::Document parsed(&alloc); | ||
| if (!parsed.Parse(value.c_str()).HasParseError() && parsed.IsObject()) { | ||
| rapidjson::Value jsonValue(parsed, alloc); | ||
| obj.AddMember(rapidjson::Value(key, alloc), jsonValue, alloc); | ||
| } else { | ||
| obj.AddMember(rapidjson::Value(key, alloc), rapidjson::Value(value.c_str(), alloc), alloc); | ||
| } | ||
| } | ||
|
|
||
| static void addOptionalModelFields(rapidjson::Value& configObj, const ModelsSettingsImpl& modelSettings, rapidjson::Document::AllocatorType& alloc) { | ||
| if (!modelSettings.batchSize.empty()) | ||
| configObj.AddMember("batch_size", rapidjson::Value(modelSettings.batchSize.c_str(), alloc), alloc); | ||
| if (!modelSettings.shape.empty()) | ||
| addJsonOrStringMember(configObj, "shape", modelSettings.shape, alloc); | ||
| if (!modelSettings.layout.empty()) | ||
| addJsonOrStringMember(configObj, "layout", modelSettings.layout, alloc); | ||
| if (modelSettings.mean.has_value()) | ||
| configObj.AddMember("mean", rapidjson::Value(modelSettings.mean.value().c_str(), alloc), alloc); | ||
| if (modelSettings.scale.has_value()) | ||
| configObj.AddMember("scale", rapidjson::Value(modelSettings.scale.value().c_str(), alloc), alloc); | ||
| if (modelSettings.colorFormat.has_value()) | ||
| configObj.AddMember("color_format", rapidjson::Value(modelSettings.colorFormat.value().c_str(), alloc), alloc); | ||
| if (modelSettings.precision.has_value()) | ||
| configObj.AddMember("precision", rapidjson::Value(modelSettings.precision.value().c_str(), alloc), alloc); | ||
| if (!modelSettings.modelVersionPolicy.empty()) | ||
| addJsonOrStringMember(configObj, "model_version_policy", modelSettings.modelVersionPolicy, alloc); | ||
| if (modelSettings.nireq != 0) | ||
| configObj.AddMember("nireq", modelSettings.nireq, alloc); | ||
| if (!modelSettings.targetDevice.empty()) | ||
| configObj.AddMember("target_device", rapidjson::Value(modelSettings.targetDevice.c_str(), alloc), alloc); | ||
| if (!modelSettings.pluginConfig.empty()) | ||
| addJsonOrStringMember(configObj, "plugin_config", modelSettings.pluginConfig, alloc); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠 Summary
Allow updating configuration for local generative models in simplified way
Allow adding to config.json classic models including their extra parameters
Corrections in --help output
🧪 Checklist
``