[WIP] Reasoning parser for LFM2.5 #4307
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial support for selecting an LFM2 reasoning parser in OVMS LLM output processing, wiring it into the runtime parser factory and Bazel build graph.
Changes:
- Register
"lfm2"as a supportedreasoningParserNameinOutputParser. - Introduce
Lfm2ReasoningParser(currently a thin wrapper overQwen3ReasoningParser) withrequiresStreamingWithSpecialTokens()enabled. - Add a Bazel
ovms_cc_librarytarget for the new header and include it inoutput_parsersdeps.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/llm/io_processing/output_parser.cpp |
Adds "lfm2" reasoning parser selection path. |
src/llm/io_processing/lfm2/lfm2_reasoning_parser.hpp |
Introduces the LFM2 reasoning parser wrapper class. |
src/llm/BUILD |
Adds/links the new parser target into the output_parsers build target. |
| } else if (reasoningParserName == "lfm2") { | ||
| reasoningParser = std::make_unique<Lfm2ReasoningParser>(tokenizer); |
mzegla
left a comment
mzegla
left a comment
There was a problem hiding this comment.
I think we mention available parsers in the docs. Please update documentation as well.
przepeck
force-pushed
the
przepeck/lfm2_reasoning_parser
branch
from
2c12a41 to
d6f1911
Compare
| protected: | ||
| static const int64_t toolCallStartTokenId = 124905; // <|tool_call_start|> | ||
| static const int64_t toolCallEndTokenId = 124906; // <|tool_call_end|> | ||
| static const int64_t reasoningEndTokenId = 124902; // </think> |
There was a problem hiding this comment.
Why do we need it? Feels like we are leaking responsibilities here.
| explicit Lfm2ToolParser(ov::genai::Tokenizer& tokenizer, int64_t botTokenId = 10, int64_t eotTokenId = 11) : | ||
| BaseOutputParser(tokenizer), botTokenId(botTokenId), eotTokenId(eotTokenId) {} |
There was a problem hiding this comment.
We are bending the LFM2 parser beyond the standard with those changes. Now some internal consts are static and some are not + the constructor is different. Not in line with other parsers.
For now I see two possibilities:
- have full implementation for LFM2.5, maybe we some extracted functions shared with LFM2, but not exact calls from parent
- make such change for all parsers (drop static fields, accept reading it from somewhere instead of hardcoded in the class)
I think option 2 would be worth considering during refactor, but since it would likely require broad correctness checks it might be quicker to go with option 1 for now.
There was a problem hiding this comment.
I will go with first option then
🛠 Summary
CVS-188930
Reasoning Parser for LFM2.5
🧪 Checklist
``