From 44a52e6f959a66410515a63c2e89e5ec3cd2b049 Mon Sep 17 00:00:00 2001 From: exzile Date: Fri, 26 Jun 2026 14:45:18 -0400 Subject: [PATCH 1/3] Add per-LLM-node generation_config.json path override Adds an optional generation_config_path field to LLMCalculatorOptions so several deployments backed by the same model weights can use different generation defaults without duplicating the model directory. Mirrors how graph_path already lets one model directory back several deployments. When unset, generation_config.json from models_path is used as before. An explicit path may be absolute or relative to models_path; a relative path is resolved against the model directory (its parent when models_path points at a file, e.g. a GGUF). An explicit path that does not exist is a load error. Shared resolveGenerationConfigPath helper is used by both the continuous batching and legacy initializers. Adds a unit test covering default, absolute, relative, and missing-path cases. Implements #4233 Co-Authored-By: Claude Opus 4.8 --- docs/llm/reference.md | 1 + .../servable_initializer.cpp | 10 ++-- .../legacy/servable_initializer.cpp | 10 ++-- src/llm/llm_calculator.proto | 7 +++ src/llm/servable_initializer.cpp | 24 ++++++++++ src/llm/servable_initializer.hpp | 1 + src/test/llm/llmnode_test.cpp | 48 +++++++++++++++++++ 7 files changed, 95 insertions(+), 6 deletions(-) diff --git a/docs/llm/reference.md b/docs/llm/reference.md index 698d05031b..3aae2d9d82 100644 --- a/docs/llm/reference.md +++ b/docs/llm/reference.md @@ -109,6 +109,7 @@ The calculator supports the following `node_options` for tuning the pipeline con - `optional string tool_parser` - name of the parser to use for tool calls extraction from model output before creating a response; - `optional bool enable_tool_guided_generation` - enable enforcing tool schema during generation. Requires setting response parser. [default = false]; - `optional SparseAttentionConfig sparse_attention_config` - Sparse attention configuration. Disabled if not specified. +- `optional string generation_config_path` - path to a `generation_config.json` holding the default generation parameters for this node. Absolute, or relative to `models_path`. When unset, `generation_config.json` from `models_path` is used. Lets several deployments backed by the same model weights use different generation defaults without duplicating the model directory. ### Caching settings The value of `cache_size` might have performance and stability implications. It is used for storing LLM model KV cache data. Adjust it based on your environment capabilities, model size and expected level of concurrency. diff --git a/src/llm/language_model/continuous_batching/servable_initializer.cpp b/src/llm/language_model/continuous_batching/servable_initializer.cpp index 1aaff99844..cdc66000bd 100644 --- a/src/llm/language_model/continuous_batching/servable_initializer.cpp +++ b/src/llm/language_model/continuous_batching/servable_initializer.cpp @@ -140,9 +140,13 @@ Status ContinuousBatchingServableInitializer::initialize(std::shared_ptr(servable->getProperties()); properties->modelsPath = parsedModelsPath; - std::filesystem::path modelGenerationConfigPath = std::filesystem::path(parsedModelsPath) / "generation_config.json"; - if (std::filesystem::exists(modelGenerationConfigPath)) { - properties->baseGenerationConfig = ov::genai::GenerationConfig(modelGenerationConfigPath.string()); + std::string generationConfigPath; + status = resolveGenerationConfigPath(generationConfigPath, parsedModelsPath, nodeOptions); + if (!status.ok()) { + return status; + } + if (std::filesystem::exists(generationConfigPath)) { + properties->baseGenerationConfig = ov::genai::GenerationConfig(generationConfigPath); } if (nodeOptions.has_tool_parser()) { properties->toolParserName = nodeOptions.tool_parser(); diff --git a/src/llm/language_model/legacy/servable_initializer.cpp b/src/llm/language_model/legacy/servable_initializer.cpp index 52d041f74d..65c371feb9 100644 --- a/src/llm/language_model/legacy/servable_initializer.cpp +++ b/src/llm/language_model/legacy/servable_initializer.cpp @@ -50,9 +50,13 @@ Status LegacyServableInitializer::initialize(std::shared_ptr& ser auto properties = std::static_pointer_cast(servable->getProperties()); properties->modelsPath = parsedModelsPath; - std::filesystem::path modelGenerationConfigPath = std::filesystem::path(parsedModelsPath) / "generation_config.json"; - if (std::filesystem::exists(modelGenerationConfigPath)) { - properties->baseGenerationConfig = ov::genai::GenerationConfig(modelGenerationConfigPath.string()); + std::string generationConfigPath; + status = resolveGenerationConfigPath(generationConfigPath, parsedModelsPath, nodeOptions); + if (!status.ok()) { + return status; + } + if (std::filesystem::exists(generationConfigPath)) { + properties->baseGenerationConfig = ov::genai::GenerationConfig(generationConfigPath); } if (nodeOptions.has_tool_parser()) { diff --git a/src/llm/llm_calculator.proto b/src/llm/llm_calculator.proto index ce252ea899..aa08b41f1e 100644 --- a/src/llm/llm_calculator.proto +++ b/src/llm/llm_calculator.proto @@ -150,4 +150,11 @@ message LLMCalculatorOptions { } optional ChatTemplateMode chat_template_mode = 26; + + // Optional path to a generation_config.json holding the default generation + // parameters for this node. Absolute, or relative to models_path. When unset, + // generation_config.json from models_path is used. Allows several deployments + // backed by the same model weights to use different generation defaults + // without duplicating the model directory. + optional string generation_config_path = 27; } diff --git a/src/llm/servable_initializer.cpp b/src/llm/servable_initializer.cpp index 90673fdbc3..7ae25bb7b8 100644 --- a/src/llm/servable_initializer.cpp +++ b/src/llm/servable_initializer.cpp @@ -334,6 +334,30 @@ Status parseModelsPath(std::string& outPath, std::string modelsPath, std::string return StatusCode::LLM_NODE_PATH_DOES_NOT_EXIST_AND_NOT_GGUFFILE; } +Status resolveGenerationConfigPath(std::string& outPath, const std::string& parsedModelsPath, const mediapipe::LLMCalculatorOptions& nodeOptions) { + // Default: generation_config.json inside the model directory. + outPath = (std::filesystem::path(parsedModelsPath) / "generation_config.json").string(); + if (!nodeOptions.has_generation_config_path() || nodeOptions.generation_config_path().empty()) { + return StatusCode::OK; + } + // Explicit per-node override. A relative path is resolved against models_path + // (its parent directory when models_path points at a file, e.g. a GGUF). + std::filesystem::path overridePath(nodeOptions.generation_config_path()); + if (overridePath.is_relative()) { + std::filesystem::path base(parsedModelsPath); + if (!std::filesystem::is_directory(base)) { + base = base.parent_path(); + } + overridePath = base / overridePath; + } + if (!std::filesystem::exists(overridePath)) { + SPDLOG_LOGGER_ERROR(modelmanager_logger, "LLM node generation_config_path: {} does not exist.", overridePath.string()); + return StatusCode::LLM_NODE_DIRECTORY_DOES_NOT_EXIST; + } + outPath = overridePath.string(); + return StatusCode::OK; +} + std::optional parseMaxModelLength(std::string& modelsPath) { std::string configPath = FileSystem::appendSlash(modelsPath) + "config.json"; std::optional maxModelLength; diff --git a/src/llm/servable_initializer.hpp b/src/llm/servable_initializer.hpp index d742db9c3e..752fcdd443 100644 --- a/src/llm/servable_initializer.hpp +++ b/src/llm/servable_initializer.hpp @@ -61,6 +61,7 @@ class GenAiServableInitializer { virtual Status initialize(std::shared_ptr& servable, const mediapipe::LLMCalculatorOptions& nodeOptions, std::string graphPath) = 0; }; Status parseModelsPath(std::string& outPath, std::string modelsPath, std::string graphPath); +Status resolveGenerationConfigPath(std::string& outPath, const std::string& parsedModelsPath, const mediapipe::LLMCalculatorOptions& nodeOptions); std::optional parseMaxModelLength(std::string& modelsPath); Status determinePipelineType(PipelineType& pipelineType, const mediapipe::LLMCalculatorOptions& nodeOptions, const std::string& graphPath); Status initializeGenAiServable(std::shared_ptr& servable, const ::mediapipe::CalculatorGraphConfig::Node& graphNodeConfig, std::string graphPath); diff --git a/src/test/llm/llmnode_test.cpp b/src/test/llm/llmnode_test.cpp index e13cf29919..3598a953a1 100644 --- a/src/test/llm/llmnode_test.cpp +++ b/src/test/llm/llmnode_test.cpp @@ -4425,6 +4425,54 @@ TEST_F(LLMVLMOptionsHttpTest, LLMVLMNodeOptionsCheckPluginConfig) { LLMNodeOptionsCheckPluginConfig(modelsPath); } +// Unit test for the per-node generation_config.json path override (issue #4233). +TEST(LLMGenerationConfigPath, ResolveGenerationConfigPath) { + std::filesystem::path base = std::filesystem::temp_directory_path() / "ovms_gencfg_test"; + std::filesystem::remove_all(base); + std::filesystem::path modelDir = base / "model"; + std::filesystem::path overrideDir = base / "overrides"; + std::filesystem::create_directories(modelDir); + std::filesystem::create_directories(overrideDir); + auto writeFile = [](const std::filesystem::path& p) { + std::ofstream ofs(p); + ofs << "{}"; + }; + writeFile(modelDir / "generation_config.json"); + writeFile(overrideDir / "custom_generation_config.json"); + + // Case 1: no override -> default generation_config.json inside the model dir. + { + mediapipe::LLMCalculatorOptions nodeOptions; + std::string outPath; + ASSERT_EQ(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::OK); + ASSERT_EQ(std::filesystem::path(outPath), modelDir / "generation_config.json"); + } + // Case 2: explicit absolute override path. + { + mediapipe::LLMCalculatorOptions nodeOptions; + nodeOptions.set_generation_config_path((overrideDir / "custom_generation_config.json").string()); + std::string outPath; + ASSERT_EQ(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::OK); + ASSERT_EQ(std::filesystem::path(outPath), overrideDir / "custom_generation_config.json"); + } + // Case 3: explicit relative override path is resolved against models_path. + { + mediapipe::LLMCalculatorOptions nodeOptions; + nodeOptions.set_generation_config_path("generation_config.json"); + std::string outPath; + ASSERT_EQ(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::OK); + ASSERT_EQ(std::filesystem::path(outPath), modelDir / "generation_config.json"); + } + // Case 4: explicit override that does not exist -> error. + { + mediapipe::LLMCalculatorOptions nodeOptions; + nodeOptions.set_generation_config_path((overrideDir / "missing.json").string()); + std::string outPath; + ASSERT_NE(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::OK); + } + + std::filesystem::remove_all(base); +} void LLMNodeOptionsCheckNonDefault(std::string& modelsPath) { std::string testPbtxt = R"( input_stream: "HTTP_REQUEST_PAYLOAD:input" From bb217bd093473bdf3c25c046cb8576d8c25b740f Mon Sep 17 00:00:00 2001 From: Joey Date: Thu, 23 Jul 2026 23:22:07 -0400 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/llm/servable_initializer.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/llm/servable_initializer.cpp b/src/llm/servable_initializer.cpp index 57c9f883e8..3aed7f65d1 100644 --- a/src/llm/servable_initializer.cpp +++ b/src/llm/servable_initializer.cpp @@ -459,11 +459,10 @@ Status resolveGenerationConfigPath(std::string& outPath, const std::string& pars } overridePath = base / overridePath; } - if (!std::filesystem::exists(overridePath)) { - SPDLOG_LOGGER_ERROR(modelmanager_logger, "LLM node generation_config_path: {} does not exist.", overridePath.string()); + if (!std::filesystem::exists(overridePath) || !std::filesystem::is_regular_file(overridePath)) { + SPDLOG_LOGGER_ERROR(modelmanager_logger, "LLM node generation_config_path: {} does not exist or is not a regular file.", overridePath.string()); return StatusCode::LLM_NODE_DIRECTORY_DOES_NOT_EXIST; } - outPath = overridePath.string(); return StatusCode::OK; } From 3d5513e43a845908255c25f78a37eb9a633089e0 Mon Sep 17 00:00:00 2001 From: exzile Date: Wed, 29 Jul 2026 00:13:25 -0400 Subject: [PATCH 3/3] Address review: reuse path util, fix override out-param, clearer status - Use FileSystem::resolvePathWithBase (added in #4385) for the relative/absolute branch instead of hand-rolling is_relative(), keeping the generic path resolution in one place. The GGUF parent-directory adjustment stays local, since the shared helper is deliberately model-agnostic. - Fix resolveGenerationConfigPath never assigning the resolved override back to outPath, so an explicit generation_config_path was validated but then discarded and the models_path default returned instead. - Report a missing/invalid override via a dedicated LLM_NODE_GENERATION_CONFIG_DOES_NOT_EXIST rather than reusing LLM_NODE_DIRECTORY_DOES_NOT_EXIST, whose "workspace path does not exist" message was misleading for this option. - Harden the unit test: unique temp directory via createTempPath with RAII cleanup (a fixed name could collide and leaked on ASSERT failure), assert the exact error code, and cover the directory-valued override and the GGUF models_path case. Co-Authored-By: Claude Opus 5 --- src/llm/servable_initializer.cpp | 14 +++++------ src/status.cpp | 1 + src/status.hpp | 1 + src/test/llm/llmnode_test.cpp | 43 +++++++++++++++++++++++++++----- 4 files changed, 45 insertions(+), 14 deletions(-) diff --git a/src/llm/servable_initializer.cpp b/src/llm/servable_initializer.cpp index 59928df93b..414fb1324f 100644 --- a/src/llm/servable_initializer.cpp +++ b/src/llm/servable_initializer.cpp @@ -453,18 +453,16 @@ Status resolveGenerationConfigPath(std::string& outPath, const std::string& pars } // Explicit per-node override. A relative path is resolved against models_path // (its parent directory when models_path points at a file, e.g. a GGUF). - std::filesystem::path overridePath(nodeOptions.generation_config_path()); - if (overridePath.is_relative()) { - std::filesystem::path base(parsedModelsPath); - if (!std::filesystem::is_directory(base)) { - base = base.parent_path(); - } - overridePath = base / overridePath; + std::filesystem::path base(parsedModelsPath); + if (!std::filesystem::is_directory(base)) { + base = base.parent_path(); } + std::filesystem::path overridePath = FileSystem::resolvePathWithBase(nodeOptions.generation_config_path(), base.string()); if (!std::filesystem::exists(overridePath) || !std::filesystem::is_regular_file(overridePath)) { SPDLOG_LOGGER_ERROR(modelmanager_logger, "LLM node generation_config_path: {} does not exist or is not a regular file.", overridePath.string()); - return StatusCode::LLM_NODE_DIRECTORY_DOES_NOT_EXIST; + return StatusCode::LLM_NODE_GENERATION_CONFIG_DOES_NOT_EXIST; } + outPath = overridePath.string(); return StatusCode::OK; } diff --git a/src/status.cpp b/src/status.cpp index 0394192e86..f6f9a67257 100644 --- a/src/status.cpp +++ b/src/status.cpp @@ -225,6 +225,7 @@ const std::unordered_map Status::statusMessageMap = { {StatusCode::LLM_NODE_NAME_ALREADY_EXISTS, "The LLM Node name is already present in nodes list"}, {StatusCode::LLM_NODE_DIRECTORY_DOES_NOT_EXIST, "The LLM Node workspace path does not exist"}, {StatusCode::LLM_NODE_PATH_DOES_NOT_EXIST_AND_NOT_GGUFFILE, "The LLM Node workspace path does not exist and not gguf model file"}, + {StatusCode::LLM_NODE_GENERATION_CONFIG_DOES_NOT_EXIST, "The LLM Node generation_config_path does not point to an existing file"}, {StatusCode::LLM_NODE_RESOURCE_STATE_INITIALIZATION_FAILED, "The LLM Node resource initialization failed"}, {StatusCode::LLM_NODE_MISSING_OPTIONS, "The LLM Node is missing options definition"}, {StatusCode::LLM_NODE_MISSING_NAME, "The LLM Node is missing name definition"}, diff --git a/src/status.hpp b/src/status.hpp index 94be7948cb..924c989ea0 100644 --- a/src/status.hpp +++ b/src/status.hpp @@ -271,6 +271,7 @@ enum class StatusCode { LLM_NODE_NAME_ALREADY_EXISTS, LLM_NODE_DIRECTORY_DOES_NOT_EXIST, LLM_NODE_PATH_DOES_NOT_EXIST_AND_NOT_GGUFFILE, + LLM_NODE_GENERATION_CONFIG_DOES_NOT_EXIST, LLM_NODE_RESOURCE_STATE_INITIALIZATION_FAILED, LLM_NODE_MISSING_OPTIONS, LLM_NODE_MISSING_NAME, diff --git a/src/test/llm/llmnode_test.cpp b/src/test/llm/llmnode_test.cpp index f9c3682ad8..3c03e68763 100644 --- a/src/test/llm/llmnode_test.cpp +++ b/src/test/llm/llmnode_test.cpp @@ -51,6 +51,7 @@ #include "../../mediapipe_internal/mediapipegraphdefinition.hpp" #include "../../ov_utils.hpp" #include "../../server.hpp" +#include "src/filesystem/filesystem.hpp" #include "src/graph_export/graph_export.hpp" #include "rapidjson/document.h" #include "rapidjson/stringbuffer.h" @@ -4504,10 +4505,25 @@ TEST_F(LLMVLMOptionsHttpTest, LLMVLMNodeOptionsCheckPluginConfig) { LLMNodeOptionsCheckPluginConfig(modelsPath); } +// RAII guard removing a temporary directory tree on scope exit, so a failed ASSERT_* +// (which returns early from the test body) does not leak the directory. +struct TempDirGuard { + std::filesystem::path path; + explicit TempDirGuard(std::filesystem::path p) : + path(std::move(p)) {} + ~TempDirGuard() { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } +}; + // Unit test for the per-node generation_config.json path override (issue #4233). TEST(LLMGenerationConfigPath, ResolveGenerationConfigPath) { - std::filesystem::path base = std::filesystem::temp_directory_path() / "ovms_gencfg_test"; - std::filesystem::remove_all(base); + // Unique per-run directory so concurrent/parallel test runs cannot collide. + std::string tempPath; + ASSERT_EQ(ovms::FileSystem::createTempPath(&tempPath), ovms::StatusCode::OK); + std::filesystem::path base(tempPath); + TempDirGuard cleanup(base); std::filesystem::path modelDir = base / "model"; std::filesystem::path overrideDir = base / "overrides"; std::filesystem::create_directories(modelDir); @@ -4518,6 +4534,8 @@ TEST(LLMGenerationConfigPath, ResolveGenerationConfigPath) { }; writeFile(modelDir / "generation_config.json"); writeFile(overrideDir / "custom_generation_config.json"); + // A GGUF "models_path" is a file, not a directory - relative overrides resolve against its parent. + writeFile(modelDir / "model.gguf"); // Case 1: no override -> default generation_config.json inside the model dir. { @@ -4542,15 +4560,28 @@ TEST(LLMGenerationConfigPath, ResolveGenerationConfigPath) { ASSERT_EQ(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::OK); ASSERT_EQ(std::filesystem::path(outPath), modelDir / "generation_config.json"); } - // Case 4: explicit override that does not exist -> error. + // Case 4: explicit override that does not exist -> dedicated error. { mediapipe::LLMCalculatorOptions nodeOptions; nodeOptions.set_generation_config_path((overrideDir / "missing.json").string()); std::string outPath; - ASSERT_NE(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::OK); + ASSERT_EQ(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::LLM_NODE_GENERATION_CONFIG_DOES_NOT_EXIST); + } + // Case 5: explicit override pointing at a directory -> same error (not a regular file). + { + mediapipe::LLMCalculatorOptions nodeOptions; + nodeOptions.set_generation_config_path(overrideDir.string()); + std::string outPath; + ASSERT_EQ(ovms::resolveGenerationConfigPath(outPath, modelDir.string(), nodeOptions), ovms::StatusCode::LLM_NODE_GENERATION_CONFIG_DOES_NOT_EXIST); + } + // Case 6: models_path points at a GGUF file - relative override resolves against its parent dir. + { + mediapipe::LLMCalculatorOptions nodeOptions; + nodeOptions.set_generation_config_path("generation_config.json"); + std::string outPath; + ASSERT_EQ(ovms::resolveGenerationConfigPath(outPath, (modelDir / "model.gguf").string(), nodeOptions), ovms::StatusCode::OK); + ASSERT_EQ(std::filesystem::path(outPath), modelDir / "generation_config.json"); } - - std::filesystem::remove_all(base); } // RAII guard that restores the global Config singleton (and optionally removes a temporary