Auto parser detect / auto input-workaround detection / --parser none#4312
Auto parser detect / auto input-workaround detection / --parser none#4312dkalinowski wants to merge 59 commits into
Conversation
dkalinowski
changed the title
There was a problem hiding this comment.
Pull request overview
Adds chat-template capability detection plus request “input workarounds” so OVMS can (a) auto-pick tool/reasoning parsers from the template and (b) normalize incoming requests to match template expectations (e.g., Gemma4 object arguments, Llama3 non-null content) across both Python/Jinja and GenAI C++ paths.
Changes:
- Introduces
ChatTemplateAnalyzer+ChatTemplateCapsand wires analysis into servable initialization for auto-detecting model family/tool parser/reasoning parser. - Adds
input_workaroundsmodule and applies it before chat template rendering (JSON for Jinja;ChatHistoryfor GenAI). - Adds unit tests + Bazel targets for analyzer/workaround behavior, and updates example templates/docs.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/llm/input_workarounds_test.cpp | New unit tests for JSON-path workarounds (object args, non-null content, applyToJson). |
| src/test/llm/chat_template_analyzer_test.cpp | New unit tests for template-family detection and inferred caps/parsers. |
| src/llm/visual_language_model/legacy/servable.cpp | Applies input workarounds to Jinja JSON path and GenAI ChatHistory path in VLM legacy flow. |
| src/llm/visual_language_model/legacy/servable_initializer.cpp | Updates chat-template mode selection (but currently duplicated). |
| src/llm/visual_language_model/continuous_batching/servable.cpp | Applies input workarounds to both Jinja JSON and GenAI ChatHistory paths in VLM CB flow. |
| src/llm/servable.hpp | Stores analyzed template caps + detected model family in GenAiServableProperties. |
| src/llm/servable.cpp | Applies workarounds before template application on both Jinja and GenAI paths. |
| src/llm/servable_initializer.cpp | Runs chat template analysis at init and auto-selects tool/reasoning parser when not explicitly configured. |
| src/llm/llm_calculator.proto | Removes an outdated TODO comment for chat template mode. |
| src/llm/language_model/legacy/servable_initializer.cpp | Updates chat-template mode selection (but currently duplicated). |
| src/llm/language_model/continuous_batching/servable_initializer.cpp | Updates chat-template mode selection (but currently duplicated). |
| src/llm/input_workarounds.hpp | Declares JSON + ChatHistory workaround APIs and aggregate apply functions. |
| src/llm/input_workarounds.cpp | Implements workarounds for JSON and ChatHistory paths (but ChatHistory mutation currently broken). |
| src/llm/chat_template_caps.hpp | New capability struct describing template requirements/support. |
| src/llm/chat_template_analyzer.hpp | Declares analyzer result + analyzer entrypoint. |
| src/llm/chat_template_analyzer.cpp | Implements Tier-1 pattern matching for family/caps/parser detection. |
| src/llm/BUILD | Adds Bazel libraries for analyzer + workarounds and links them into genai servables. |
| src/BUILD | Adds a new test library for analyzer/workaround tests into the main test target. |
| extras/chat_template_examples/chat_template_qwen36.jinja | Updates example template to from_json tool-call arguments before iterating. |
| extras/chat_template_examples/chat_template_qwen35.jinja | Adds a new example template variant with the same from_json workaround. |
| docs/plan_chat_template_input_workarounds.md | Adds a design/plan document describing detection/workaround strategy and roadmap. |
| result.caps.supportsToolCalls = true; | ||
| result.caps.supportsTools = true; | ||
| result.caps.supportsToolResponses = true; | ||
| return result; |
There was a problem hiding this comment.
TODO: Add reasoning parser once @przepeck implements it
| } else if (analysisResult.detectedToolParser.has_value()) { | ||
| properties->toolParserName = analysisResult.detectedToolParser.value(); | ||
| SPDLOG_LOGGER_INFO(llm_calculator_logger, "Auto-detected tool_parser: {}", properties->toolParserName); | ||
| } |
There was a problem hiding this comment.
What if user for some reason don't want to use toolparser with model?
I remember, we were talking about CLI option --tool_parser none, don't we want to do it in this PR?
There was a problem hiding this comment.
it should be possible to set --tool_parser ""
i will verify and add tests, good catch
|
|
||
| try { | ||
| strOk = PyJinjaTemplateProcessor::applyChatTemplate(templateProcessor, strArgsJson, strOut); | ||
| } catch (...) { |
There was a problem hiding this comment.
Is it always safe to just move on from exception? Could error in applyChatTemplate corrupt the state and second call that is done below would crash?
There was a problem hiding this comment.
Neither of workarounds rely on exception check from jinja. Jinja doesnt throw an exception if it cannot apply the template. Either way, I added try catch. Will add error logs here just in case it changes in the future
| #if (PYTHON_DISABLE == 0) | ||
| if (properties->chatTemplateMode != ChatTemplateMode::JINJA) { | ||
| #endif | ||
| if (!probeChatTemplateBasicRenderMinja(properties->tokenizer)) { | ||
| SPDLOG_LOGGER_ERROR(llm_calculator_logger, "Chat template is not compatible with minja — basic rendering failed. " | ||
| "Disabling /chat/completions endpoint for this model."); | ||
| properties->tokenizer.set_chat_template(""); | ||
| return; | ||
| } | ||
| #if (PYTHON_DISABLE == 0) | ||
| } | ||
| #endif |
There was a problem hiding this comment.
IMHO it's not a very readable construction with those ifdefs. Maybe we could determine if minja should be used earlier and just rely on the simple flag?
There was a problem hiding this comment.
btw. what about the jinja path? Is it used here?
There was a problem hiding this comment.
IMHO it's not a very readable construction with those ifdefs. Maybe we could determine if minja should be used earlier and just rely on the simple flag?
I think @rasapala is working on that with runtime python check
btw. what about the jinja path? Is it used here?
No, we dont need basic probe for jinja, jinja doesnt silently fail
dkalinowski
changed the title
|
|
||
| // ============================================================================= | ||
| // Same story as Qwen3-8B. | ||
| // TODO(mzeglars): Do we keep it? |
There was a problem hiding this comment.
Don't think so. If qwen3-8B has the same type of template (in terms of needed adaptations) and we have separate tests in chat templates dir, then I suppose we can drop it here in the end to end test. Unless we wanted to check some VLM specific capabilities.
przepeck
left a comment
przepeck
left a comment
There was a problem hiding this comment.
could you add to the docs/parameters.md information about --tool_parser none option?
| #include <filesystem> | ||
| #include <chrono> | ||
| #include <future> | ||
| #include <memory> |
| // detect what the template requires. This is model-agnostic and tests actual | ||
| // template behavior rather than relying solely on string pattern matching. | ||
| // Probes for: | ||
| // - requiresObjectArguments (workaround: string→object conversion of tool_call arguments) | ||
| // - requiresNonNullContent (workaround: ensure content="" rather than content=null for tool_call messages) |
| // For now the capabilities are only related to tool calls, therefore we early exit here. | ||
| // In the future, if we add more capabilities to probe, we will need to remove this early exit and probe for those capabilities as well. | ||
| if (!caps.supportsToolCalls) { | ||
| return true; | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.