Conversation
Resolved 5 conflicts: - versions.mk: kept onyx patch-required OV_SOURCE/OV_GENAI pins; advanced OV_TOKENIZERS to the newer upstream commit (safe: tokenizers built separately with BUILD_TOKENIZERS=OFF). - src/llm/BUILD, output_parser.cpp, parser_config_validation.cpp, chat_template_end_to_end_jinja_test.cpp: additive unions keeping both the onyx parsers/tests and the incoming lfm2.5 / minicpm5 parsers/tests. Unified onyx_tool_parser onto the shared helpers introduced by the incoming branch: dropped its private parseToolSchema / createToolsParametersTypesMap (now in base_output_parser) and trimNewline / jsonTypeOf / enforceStringValue (now in io_processing/utils), which otherwise collided as static-follows-extern redeclarations. Builds clean: bazel build //src:ovms //src:ovms_test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[Integration branch] changed install deps script to use private repos
[Integration branch] Merging parser into Integration branch
…aming Lookup content streaming
There was a problem hiding this comment.
Pull request overview
Adds Onyx/MuseSpark tool-call and reasoning parsing with ATEM framing, template detection, streaming support, and tests.
Changes:
- Adds Onyx tool and reasoning parsers.
- Adds Onyx chat templates and automatic detection.
- Adds parser, template, streaming, and round-trip tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/llm/io_processing/onyx/onyx_tool_parser.hpp |
Declares Onyx tool parsing. |
src/llm/io_processing/onyx/onyx_tool_parser.cpp |
Implements ATEM parsing and streaming. |
src/llm/io_processing/onyx/onyx_reasoning_parser.hpp |
Declares reasoning parsing. |
src/llm/io_processing/onyx/onyx_reasoning_parser.cpp |
Implements reasoning extraction. |
src/llm/io_processing/output_parser.hpp |
Adds special-token decoding state. |
src/llm/io_processing/output_parser.cpp |
Integrates Onyx parsers. |
src/llm/io_processing/parser_config_validation.cpp |
Registers Onyx parser names. |
src/llm/io_processing/chat_template/analyzer.cpp |
Detects Onyx templates. |
src/llm/io_processing/chat_template/probe.cpp |
Recognizes ATEM parameters. |
src/llm/BUILD |
Adds Onyx parser targets. |
src/BUILD |
Adds round-trip test dependencies. |
extras/chat_template_examples/chat_template_onyx.jinja |
Adds example Onyx template. |
src/test/llm/chat_templates/chat_template_onyx.jinja |
Adds Onyx test template. |
src/test/llm/output_parsers/onyx_output_parser_test.cpp |
Tests parser behavior and streaming. |
src/test/llm/chat_template_and_parser_onyx_roundtrip_test.cpp |
Tests template/parser round trips. |
src/test/llm/chat_template_end_to_end_minja_test.cpp |
Adds Minja template tests. |
src/test/llm/chat_template_end_to_end_jinja_test.cpp |
Adds Jinja template tests. |
src/test/llm/chat_template_analyzer_test.cpp |
Tests Onyx detection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (paramIt != this->toolsParametersTypeMap.end()) { | ||
| auto paramJt = paramIt->second.find(currentParameterName); | ||
| if (paramJt != paramIt->second.end() && (paramJt->second == ParameterType::BOOLEAN)) { | ||
| std::transform(parameterValueAsString.begin(), parameterValueAsString.end(), parameterValueAsString.begin(), ::tolower); |
There was a problem hiding this comment.
Sounds good, especially if we already have a function used in another parser
mzegla
left a comment
There was a problem hiding this comment.
Since we have confirmations about this implementation working in demos and getting good accuracy results I approve this - but note the comments. We should plan addressing any doubtful parts as soon as possible.
| // TODO @atobiszei we need to stream between recipient=self & <eom> | ||
| // TODO: streaming support is a first draft. It only forwards the chunk as | ||
| // reasoning_content once we've seen the "to=self" start tag; stripping the | ||
| // generic final-answer envelope (Case 3 above) in streaming mode is not | ||
| // implemented yet and needs its own design (the envelope prefix/suffix can | ||
| // straddle multiple chunks). |
There was a problem hiding this comment.
Are those TODOs resolved? If so, we should remove it, otherwise, let's plan addressing them.
| if (paramIt != this->toolsParametersTypeMap.end()) { | ||
| auto paramJt = paramIt->second.find(currentParameterName); | ||
| if (paramJt != paramIt->second.end() && (paramJt->second == ParameterType::BOOLEAN)) { | ||
| std::transform(parameterValueAsString.begin(), parameterValueAsString.end(), parameterValueAsString.begin(), ::tolower); |
There was a problem hiding this comment.
Sounds good, especially if we already have a function used in another parser
🛠 Summary
CVS-189675
New tool parser for Meta/MuseSpark
🧪 Checklist
``