diff --git a/common/chat-auto-parser-generator.cpp b/common/chat-auto-parser-generator.cpp index db3a6cc6fe3..bcde62be077 100644 --- a/common/chat-auto-parser-generator.cpp +++ b/common/chat-auto-parser-generator.cpp @@ -103,6 +103,10 @@ common_chat_params peg_generator::generate_parser(const common_chat_template & data.grammar_triggers = { { COMMON_GRAMMAR_TRIGGER_TYPE_WORD, trigger_marker } }; + if (autoparser.tools.format.openai_wrapper_trigger) { + // model emits the OpenAI function wrapper, trigger on it + data.grammar_triggers.push_back({ COMMON_GRAMMAR_TRIGGER_TYPE_WORD, "{\"type\": \"function\"," }); + } } } @@ -224,13 +228,13 @@ common_peg_parser analyze_tools::build_tool_parser_json_native(parser_build_cont auto single_tool_parser = p.standard_json_tools( format.per_call_start, format.per_call_end, inputs.tools, inputs.parallel_tool_calls, inputs.tool_choice == COMMON_CHAT_TOOL_CHOICE_REQUIRED, name_field, args_field, format.tools_array_wrapped, - format.fun_name_is_key, format.id_field, format.gen_id_field, format.parameter_order); + format.fun_name_is_key, format.id_field, format.gen_id_field, format.parameter_order, format.openai_wrapper_trigger); tools_parser = p.trigger_rule("tool-calls", p.one_or_more(single_tool_parser + p.space())); } else { tools_parser = p.standard_json_tools( format.section_start, format.section_end, inputs.tools, inputs.parallel_tool_calls, inputs.tool_choice == COMMON_CHAT_TOOL_CHOICE_REQUIRED, name_field, args_field, format.tools_array_wrapped, - format.fun_name_is_key, format.id_field, format.gen_id_field, format.parameter_order); + format.fun_name_is_key, format.id_field, format.gen_id_field, format.parameter_order, format.openai_wrapper_trigger); } // Handle content wrappers if present diff --git a/common/chat-auto-parser.h b/common/chat-auto-parser.h index 7858f6572f2..9e8113f2442 100644 --- a/common/chat-auto-parser.h +++ b/common/chat-auto-parser.h @@ -181,6 +181,7 @@ struct tool_format_analysis { bool fun_name_is_key = false; // In JSON format function name is JSON key, i.e. { "": { ... arguments ... } } bool tools_array_wrapped = false; // Tool calls wrapped in JSON array [...] + bool openai_wrapper_trigger = false; // model emits the OpenAI function wrapper, trigger on it std::string function_field = "function"; std::string name_field = "name"; diff --git a/common/chat-diff-analyzer.cpp b/common/chat-diff-analyzer.cpp index 0875c5347f4..96eadfc9a6c 100644 --- a/common/chat-diff-analyzer.cpp +++ b/common/chat-diff-analyzer.cpp @@ -165,6 +165,14 @@ static std::vector void { + if (tmpl.src.find("Respond in the format {\"name\": function name") != std::string::npos && + tmpl.src.find("Do not use variables.") != std::string::npos) { + analysis.tools.format.openai_wrapper_trigger = true; + LOG_DBG(ANSI_ORANGE "[Patch: JSON name/parameters tool instruction]\n" ANSI_RESET); + } + }, }); diff --git a/common/chat-peg-parser.cpp b/common/chat-peg-parser.cpp index 9bc5ac98be6..ce7a8f41968 100644 --- a/common/chat-peg-parser.cpp +++ b/common/chat-peg-parser.cpp @@ -745,7 +745,8 @@ common_peg_parser common_chat_peg_builder::build_json_tools_flat_keys( const std::string & effective_args_key, const std::string & call_id_key, const std::string & gen_call_id_key, - const std::vector & parameters_order) { + const std::vector & parameters_order, + bool accept_openai_wrapper) { auto tool_choices = choice(); auto name_key_parser = literal("\"" + effective_name_key + "\""); @@ -807,7 +808,13 @@ common_peg_parser common_chat_peg_builder::build_json_tools_flat_keys( return idx_a < idx_b; }); - auto ordered_body = tool_open(literal("{")) + space(); + // accept an optional leading "type": "function" field when the model emits the OpenAI wrapper + common_peg_parser type_field = eps(); + if (accept_openai_wrapper) { + type_field = optional(literal("\"type\"") + space() + literal(":") + space() + + literal("\"function\"") + space() + literal(",") + space()); + } + auto ordered_body = tool_open(literal("{")) + space() + type_field; for (size_t i = 0; i < parser_pairs.size(); i++) { ordered_body = ordered_body + parser_pairs[i].first; if (i < parser_pairs.size() - 1) { @@ -870,7 +877,8 @@ common_peg_parser common_chat_peg_builder::standard_json_tools( bool function_is_key, const std::string & call_id_key, const std::string & gen_call_id_key, - const std::vector & parameters_order) { + const std::vector & parameters_order, + bool accept_openai_wrapper) { if (!tools.is_array() || tools.empty()) { return eps(); } @@ -888,7 +896,7 @@ common_peg_parser common_chat_peg_builder::standard_json_tools( if (!name_spec.first.empty() || !args_spec.first.empty()) { tool_choices = build_json_tools_nested_keys(tools, effective_name_key, effective_args_key, call_id_key, gen_call_id_key); } else { - tool_choices = build_json_tools_flat_keys(tools, effective_name_key, effective_args_key, call_id_key, gen_call_id_key, parameters_order); + tool_choices = build_json_tools_flat_keys(tools, effective_name_key, effective_args_key, call_id_key, gen_call_id_key, parameters_order, accept_openai_wrapper); } } diff --git a/common/chat-peg-parser.h b/common/chat-peg-parser.h index a4643fbea86..b3ffd7de2dd 100644 --- a/common/chat-peg-parser.h +++ b/common/chat-peg-parser.h @@ -120,7 +120,8 @@ class common_chat_peg_builder : public common_peg_parser_builder { bool function_is_key = false, const std::string & call_id_key = "", const std::string & gen_call_id_key = "", - const std::vector & parameters_order = {}); + const std::vector & parameters_order = {}, + bool accept_openai_wrapper = false); // Legacy-compatible helper for building XML/tagged style tool calls // Used by tests and manual parsers @@ -157,7 +158,8 @@ class common_chat_peg_builder : public common_peg_parser_builder { const std::string & effective_args_key, const std::string & call_id_key, const std::string & gen_call_id_key, - const std::vector & parameters_order); + const std::vector & parameters_order, + bool accept_openai_wrapper); }; inline common_peg_arena build_chat_peg_parser( diff --git a/common/chat.cpp b/common/chat.cpp index 24e58ab0640..eadf0e7abe7 100644 --- a/common/chat.cpp +++ b/common/chat.cpp @@ -2520,8 +2520,10 @@ common_chat_msg common_chat_peg_parse(const common_peg_arena & src_pars } return msg; } - throw std::runtime_error(std::string("Failed to parse input at pos ") + std::to_string(result.end) + ": " + - effective_input.substr(result.end)); + LOG_WRN("%s: unparsed %s output: %s\n", __func__, common_chat_format_name(params.format), + effective_input.substr(result.end).c_str()); + throw std::runtime_error(std::string("The model produced output that does not match the expected ") + + common_chat_format_name(params.format) + " format"); } common_chat_msg msg; diff --git a/common/json-schema-to-grammar.cpp b/common/json-schema-to-grammar.cpp index e2c4d6ce22e..542a627a983 100644 --- a/common/json-schema-to-grammar.cpp +++ b/common/json-schema-to-grammar.cpp @@ -376,6 +376,29 @@ class common_schema_converter { return _add_rule("dot", rule); }; + // Translates a PCRE-style shorthand character-class escape (\\d, \\D, \\w, \\W, \\s, \\S) + // that appears OUTSIDE of a [...] bracket expression into its own standalone GBNF + // character-class rule (analogous to get_dot() above for '.'). Unlike a shorthand escape + // mixed inside a [...] class, a standalone shorthand escape (positive or negated) always + // has a clean, exact GBNF translation. + auto get_shorthand_class = [&](char esc) -> std::string { + switch (esc) { + case 'd': return _add_rule("d", "[0-9]"); + case 'D': return _add_rule("not-d", "[^0-9]"); + case 'w': return _add_rule("w", "[A-Za-z0-9_]"); + case 'W': return _add_rule("not-w", "[^A-Za-z0-9_]"); + case 's': return _add_rule("s", "[ \\t\\n\\r]"); + case 'S': return _add_rule("not-s", "[^ \\t\\n\\r]"); + default: + // unreachable: only invoked for d/D/w/W/s/S, see dispatch below. + _errors.push_back("Unsupported shorthand escape '\\" + std::string(1, esc) + "' in pattern '" + pattern + "'"); + return _add_rule("unknown-escape", "[]"); + } + }; + auto is_shorthand_class = [](char c) { + return c == 'd' || c == 'D' || c == 'w' || c == 'W' || c == 's' || c == 'S'; + }; + // Joins the sequence, merging consecutive literals together. auto join_seq = [&]() { std::vector ret; @@ -414,6 +437,12 @@ class common_schema_converter { if (c == '.') { seq.emplace_back(get_dot(), false); i++; + } else if (c == '\\' && i + 1 < length && is_shorthand_class(sub_pattern[i + 1])) { + // Standalone \\d, \\D, \\w, \\W, \\s, \\S (i.e. not nested inside a [...] class): + // translate to their own GBNF character-class rule so this token composes with + // quantifiers (*, +, ?, {m,n}) exactly like get_dot()'s "." handling above. + seq.emplace_back(get_shorthand_class(sub_pattern[i + 1]), false); + i += 2; } else if (c == '(') { i++; if (i < length && sub_pattern[i] == '?') { @@ -447,9 +476,47 @@ class common_schema_converter { std::string square_brackets = std::string(1, c); i++; while (i < length && sub_pattern[i] != ']') { - if (sub_pattern[i] == '\\') { - square_brackets += sub_pattern.substr(i, 2); - i += 2; + if (sub_pattern[i] == '\\' && i + 1 < length) { + char esc = sub_pattern[i + 1]; + // PCRE shorthand classes have no GBNF escape of their own (GBNF's [...] only + // understands literal chars, ranges, and its own \\x/\\u/\\U/\\t/\\r/\\n/\\\\/\\"/\\[/\\] + // escapes) -- src/llama-grammar.cpp's parse_char() throws "unknown escape" on + // anything else. \\d/\\w/\\s are positive classes, so they can always be + // inlined as extra members of this (possibly mixed) [...] class. \\D/\\W/\\S + // are negated classes: inlining them alongside other members of a positive + // class has no single-range GBNF equivalent (e.g. [\\D2468] can't be expressed + // as one flat GBNF character class), so fail loudly here at schema-conversion + // time -- with the offending pattern named -- rather than emitting grammar text + // that will only fail later, without context, inside the GBNF parser. + switch (esc) { + case 'd': + square_brackets += "0-9"; + i += 2; + break; + case 'w': + square_brackets += "A-Za-z0-9_"; + i += 2; + break; + case 's': + square_brackets += " \\t\\n\\r"; + i += 2; + break; + case 'D': + case 'W': + case 'S': + _errors.push_back( + "Pattern '" + pattern + "': negated shorthand class '\\" + std::string(1, esc) + + "' is not supported inside a [...] character class (GBNF has no single-range " + "equivalent for a negated shorthand mixed with other class members); rewrite " + "the pattern to use '\\" + std::string(1, esc) + "' on its own outside of " + "brackets, or replace it with an explicit negated range, e.g. [^0-9] for \\D"); + i += 2; + break; + default: + square_brackets += sub_pattern.substr(i, 2); + i += 2; + break; + } } else { square_brackets += sub_pattern[i]; i++; @@ -525,6 +592,13 @@ class common_schema_converter { while (i < length) { if (sub_pattern[i] == '\\' && i < length - 1) { char next = sub_pattern[i + 1]; + if (is_shorthand_class(next)) { + // Don't swallow \d/\D/\w/\W/\s/\S into the literal run: break out + // (flushing whatever literal text was collected so far) and let the + // outer dispatch loop's shorthand-class branch above handle it as its + // own token, the same way it would if it started the token. + break; + } if (ESCAPED_IN_REGEXPS_BUT_NOT_IN_LITERALS.find(next) != ESCAPED_IN_REGEXPS_BUT_NOT_IN_LITERALS.end()) { i++; literal += sub_pattern[i]; diff --git a/common/sampling.cpp b/common/sampling.cpp index 85f8ed50b35..3345c53e139 100644 --- a/common/sampling.cpp +++ b/common/sampling.cpp @@ -259,6 +259,9 @@ struct common_sampler * common_sampler_init(const struct llama_model * model, st } } } + if (!grmr && !grammar_str.empty()) { + throw std::runtime_error("failed to parse grammar"); + } // Compute prefill tokens from the generation prompt std::vector prefill_tokens; diff --git a/tests/test-json-schema-to-grammar.cpp b/tests/test-json-schema-to-grammar.cpp index b4362852c39..64c17c8daa0 100755 --- a/tests/test-json-schema-to-grammar.cpp +++ b/tests/test-json-schema-to-grammar.cpp @@ -1538,8 +1538,20 @@ int main() { tc.verify_status(FAILURE); } }; + // Same as `run`, but for SUCCESS cases also feeds the generated grammar through + // src/llama-grammar.cpp's own parser (llama_grammar_parser::parse) to confirm it is + // valid, well-formed GBNF -- not just that the converter didn't throw. This matters + // in particular for the PCRE shorthand character-class (\d, \w, \s, ...) translation + // below, since the whole point of that fix is that llama-grammar.cpp's parser used to + // reject the untranslated escapes at grammar-parse time. + auto run_and_check_gbnf = [&](const TestCase & tc) { + run(tc); + if (tc.expected_status == SUCCESS) { + tc.verify_expectation_parseable(); + } + }; - run({ + run_and_check_gbnf({ SUCCESS, "regexp with non-capturing group", R"""({ @@ -1552,7 +1564,7 @@ int main() { )""", }); - run({ + run_and_check_gbnf({ SUCCESS, "regexp with nested non-capturing groups", R"""({ @@ -1564,6 +1576,133 @@ int main() { space ::= | " " | "\n"{1,2} [ \t]{0,20} )""", }); + + // Regression coverage for the PCRE shorthand character-class escapes (\d, \D, \w, \W, + // \s, \S) that JSON Schema `pattern` regexes commonly use but that GBNF has no native + // escape for: src/llama-grammar.cpp's parse_char() throws "unknown escape" if one reaches + // it untranslated. A single such pattern anywhere in a combined tool-calling grammar used + // to disable grammar-constrained decoding for the whole request (confirmed against a + // PagerDuty `create_schedule` MCP tool schema using a leap-year-validated ISO-8601 + // `pattern`, e.g. containing "\d\d[2468][048]", that reliably logged + // "parse: error parsing grammar: unknown escape at \d\d..."). + + run_and_check_gbnf({ + SUCCESS, + "regexp with \\d \\w \\s shorthand classes inside [...] (mixed with literal members)", + R"""({ + "type": "string", + "pattern": "^[\\dA-F]{4}-[\\w.-]+ [\\s,;]$" + })""", + R"""( + root ::= "\"" (root-1{4,4} "-" [A-Za-z0-9_.-]+ " " [ \t\n\r,;]) "\"" space + root-1 ::= [0-9A-F] + space ::= | " " | "\n"{1,2} [ \t]{0,20} + )""", + }); + + run_and_check_gbnf({ + SUCCESS, + "regexp with standalone \\d \\w \\s shorthand classes and quantifiers", + R"""({ + "type": "string", + "pattern": "^\\d+\\.\\w+\\s?$" + })""", + R"""( + d ::= [0-9] + root ::= "\"" (d+ "." w+ s?) "\"" space + s ::= [ \t\n\r] + space ::= | " " | "\n"{1,2} [ \t]{0,20} + w ::= [A-Za-z0-9_] + )""", + }); + + run_and_check_gbnf({ + SUCCESS, + "regexp with standalone negated shorthand classes \\D \\W \\S", + R"""({ + "type": "string", + "pattern": "^\\D\\W\\S$" + })""", + R"""( + not-d ::= [^0-9] + not-s ::= [^ \t\n\r] + not-w ::= [^A-Za-z0-9_] + root ::= "\"" (not-d not-w not-s) "\"" space + space ::= | " " | "\n"{1,2} [ \t]{0,20} + )""", + }); + + run_and_check_gbnf({ + SUCCESS, + "regexp shaped like an ISO-8601 date (\\d immediately after literal '-', " + "exercising \\d recognition mid-literal-run, not just at the start of a token)", + R"""({ + "type": "string", + "pattern": "^\\d{4}-\\d{2}-\\d{2}$" + })""", + R"""( + d ::= [0-9] + root ::= "\"" (root-1{4,4} "-" root-1{2,2} "-" root-1{2,2}) "\"" space + root-1 ::= d + space ::= | " " | "\n"{1,2} [ \t]{0,20} + )""", + }); + + run_and_check_gbnf({ + SUCCESS, + "regexp shaped like the PagerDuty leap-year-validated ISO-8601 pattern that broke " + "grammar-constrained decoding in production (\\d\\d[2468][048] etc, top-level " + "alternation)", + R"""({ + "type": "string", + "pattern": "^\\d\\d[2468][048]|\\d\\d[13579][26]|\\d\\d0[48]$" + })""", + R"""( + d ::= [0-9] + root ::= "\"" (d d [2468] [048] | d d [13579] [26] | d d "0" [48]) "\"" space + space ::= | " " | "\n"{1,2} [ \t]{0,20} + )""", + }); + + // \D, \W, \S mixed inside a [...] class alongside other members have no clean + // single-range GBNF translation (a positive class can't compose with a negated-shorthand + // member), so conversion must fail loudly and *name the offending pattern* right here at + // schema-conversion time, rather than emitting grammar text that only fails later -- + // without any schema context -- inside llama-grammar.cpp's parser. + run({ + FAILURE, + "regexp with \\D negated shorthand mixed inside [...] must fail at conversion time, not silently produce invalid GBNF", + R"""({ + "type": "string", + "pattern": "^[\\D!?]+$" + })""", + "" + }); + + // Verify the failure above is a clean, actionable error (names the pattern and the + // offending escape), not a generic/opaque failure -- this is the property that makes it + // debuggable at conversion time instead of a bare "unknown escape" from the GBNF parser + // with no indication of which schema or pattern caused it. + { + const std::string bad_pattern = "^[\\D!?]+$"; + bool threw = false; + try { + json_schema_to_grammar(nlohmann::ordered_json::parse( + R"({"type": "string", "pattern": "^[\\D!?]+$"})"), true); + } catch (const std::invalid_argument & ex) { + threw = true; + std::string msg = ex.what(); + fprintf(stderr, "- negated shorthand class error message: %s\n", msg.c_str()); + if (msg.find(bad_pattern) == std::string::npos || msg.find("\\D") == std::string::npos) { + fprintf(stderr, "# FAILED: error message does not name the offending pattern/escape:\n%s\n", msg.c_str()); + assert(false); + } + } + if (!threw) { + fprintf(stderr, "# FAILED: expected a negated shorthand class mixed inside [...] to raise std::invalid_argument\n"); + assert(false); + } + } } if (getenv("LLAMA_SKIP_TESTS_SLOW_ON_EMULATOR")) { diff --git a/tools/server/tests/unit/test_chat_completion.py b/tools/server/tests/unit/test_chat_completion.py index fe55dc5ab17..b00aac649d7 100644 --- a/tools/server/tests/unit/test_chat_completion.py +++ b/tools/server/tests/unit/test_chat_completion.py @@ -307,6 +307,20 @@ def test_completion_with_grammar(jinja: bool, grammar: str, n_predicted: int, re assert match_regex(re_content, choice["message"]["content"]), choice["message"]["content"] +def test_completion_with_invalid_grammar(): + global server + server.start() + res = server.make_request("POST", "/chat/completions", data={ + "max_tokens": 8, + "messages": [ + {"role": "user", "content": "Does not matter what I say, does it?"}, + ], + "grammar": "root ::= this is (not valid GBNF", + }) + assert res.status_code == 400, res.body + assert "error" in res.body + + @pytest.mark.parametrize("messages", [ None, "string",