common/grammar : replace problematic backtracking regex [\s\S]*#18342
common/grammar : replace problematic backtracking regex [\s\S]*#18342aldehir merged 6 commits intoggml-org:masterfrom
[\s\S]*#18342Conversation
|
@aldehir A bit off-topic, but I played with your test suite and noticed that Qwen3 models are failing some of the agentic tests: ./bin/llama-server -hf ggml-org/Qwen3-0.6B-GGUF
./llm-serve-test --base-url http://localhost:8080/v1 --model qwen3-0.6b --filter agentic
Agentic
✗ agentic_tool_call - turn 2 request failed: unexpected status 500: {"error":{"code":500,"message":"Trying to call method 'lstrip' on null at row 41, column 128:\n
✗ agentic_reasoning_in_template - /apply-template failed: unexpected status 500: {"error":{"code":500,"message":"Trying to call method 'lstrip' on null at row 41, column 128:\n
✓ agentic_reasoning_not_in_user_template (303ms)
Results: 1/3 passedSeems like we are calling 41c41
< {{- '<|im_start|>' + message.role + '\n<think>\n' + reasoning_content.strip('\n') + '\n</think>\n\n' + content.lstrip('\n') }}
---
> {{- '<|im_start|>' + message.role + '\n<think>\n' + reasoning_content.strip('\n') + '\n</think>\n\n' + content }}Is this a bug in the chat template of these models? |
Ah, looks like Minja is getting a false negative on the "requires non-null content" check. Since that code path is only hit when I can submit a Minja PR, or we can convert null content to an empty string by default. I don't know why we don't already, seems like it makes the most sense. |
ggerganov
left a comment
There was a problem hiding this comment.
Approving based on feedback #18188 (comment)
|
Just wanted to report: Commit works fine with tool calling using MiniMax M2.1. Thanks a lot! 🙏 The backtracking regex has bitten me quite a few times in the past. |
…8342) * grammar : add support for std::regex_search() with trigger patterns * common : update hermes2 pro trigger to search instead of match * common : use regex_search with anchoring for partial matching * common : adjust regex partial tests to use new pattern * grammar : check pattern directly instead of adding a type * common : adjust existing patterns to match new semantics
There are several regex patterns scattered throughout the codebase that use
[\s\S]*or[\s\S]*?. This is problematic because it matches characters recursively in backtracking engines such asstd::regex, causing stack overflows on large inputs.Minimal reproducing example
This PR replaces these instances with alternative solutions.
Fixes #17902, #18188
Details
src/llama-grammar.cpp
These patterns exist because the grammar sampler uses
std::regex_match(). The only way to search for a needle is to surround it with[\s\S]*?<pat>[\s\S]*.To address this, I check whether the pattern is wrapped in anchors (
^$). If not,std::regex_search()is used instead. Most engines search by iteratively matching at each position, starting from the beginning. This is an improvement over recursively matching characters.common/sampling.cpp
To accommodate the grammar change,
COMMON_TRIGGER_TYPE_PATTERN_FULLnow wraps the pattern as^<pat>$. The problematic patterns are removed forCOMMON_TRIGGER_TYPE_PATTERNandCOMMON_TRIGGER_TYPE_WORD. This maintains existing semantics to remain compatible with chat parsing implementations.Additionally,
COMMON_TRIGGER_TYPE_PATTERNno longer adds an implicit capture group. The capture semantics now align withCOMMON_TRIGGER_TYPE_PATTERN_FULLand can be used to determine the start of the input fed to the grammar sampler.common/chat.cpp
As an example, I removed
[\s\S]*?from the Hermes 2 Pro implementation. I also removed"(?:<think>[\\s\\S]*?</think>\\s*)?"since it's optional and non-capturing—it would be consumed by[\s\S]*?anyway and isn't needed when usingstd::regex_search().I updated existing
COMMON_TRIGGER_TYPE_PATTERNpatterns to use non-capturing groups, since it now follows the same capture semantics asCOMMON_TRIGGER_TYPE_PATTERN_FULL.common/regex-partial.cpp
The generated reverse regex pattern contains a trailing
[\s\S]*. This is replaced by anchoring the generated pattern as^<pat>and usingstd::regex_constants::match_continuousto enforce matching at the start of the reverse iterator.Models Tested
I tested content, reasoning, tool calling, and agentic scenarios with my own test suite on the following models:
tool_choice = requiredis broken but this is an existing issue@ochafik If you have some time, I'd appreciate your opinion on these changes.
No AI was used to code, only to review changes.