Skip to content

Commit 2233737

Browse files
aldehiraldehir
authored
common : add commentary rules for gpt-oss-20b (#21286)
1 parent e15efe0 commit 2233737

4 files changed

Lines changed: 124 additions & 16 deletions

File tree

‎common/chat.cpp‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -980,23 +980,27 @@ static common_chat_params common_chat_params_init_gpt_oss(const common_chat_temp
980980
auto channel = p.literal("<|channel|>") + (p.literal("commentary") | p.literal("analysis"));
981981
auto constrain_type = p.chars("[A-Za-z0-9_-]", 1, -1);
982982

983+
// Occasionally, gpt-oss-20b will prefix channels with this commentary
984+
auto stray_commentary = p.optional(p.literal("<|channel|>commentary") + p.optional(p.literal(" to=assistant")));
985+
auto start_analysis = stray_commentary + p.literal("<|channel|>analysis<|message|>");
986+
983987
if (extract_reasoning) {
984-
p.rule("analysis", p.literal("<|channel|>analysis<|message|>") + p.reasoning(content) + end);
988+
p.rule("analysis", start_analysis + p.reasoning(content) + end);
985989
} else {
986-
p.rule("analysis", p.content(p.literal("<|channel|>analysis<|message|>") + content + end));
990+
p.rule("analysis", p.content(start_analysis + content + end));
987991
}
988992

989993
auto analysis = p.ref("analysis");
990994
auto preamble = p.rule("preamble", p.literal("<|channel|>commentary<|message|>") + p.content(content) + end);
991-
auto final_msg = p.rule("final", p.literal("<|channel|>final<|message|>") + p.content(content));
995+
auto final_msg = p.rule("final", stray_commentary + p.literal("<|channel|>final<|message|>") + p.content(content));
992996

993997
// Consume any unsolicited tool calls, e.g. builtin functions
994998
auto unsolicited = p.rule("unsolicited", p.atomic(p.optional(channel) + p.literal(" to=") + content + end));
995999

9961000
auto any = p.rule("any", preamble | analysis);
9971001

9981002
if (has_response_format) {
999-
auto constraint = p.optional(p.space() + p.literal("<|constrain|>") + constrain_type);
1003+
auto constraint = p.optional(p.space() + p.optional(p.literal("<|constrain|>")) + constrain_type);
10001004
auto response_format = p.rule("response-format",
10011005
p.literal("<|channel|>final") + constraint + p.literal("<|message|>") +
10021006
p.content(p.schema(p.json(), "response-format-schema", inputs.json_schema)));
@@ -1013,7 +1017,7 @@ static common_chat_params common_chat_params_init_gpt_oss(const common_chat_temp
10131017
const auto & params = function.at("parameters");
10141018

10151019
auto func_name = p.literal(" to=functions.") + p.tool_name(p.literal(name));
1016-
auto constraint = p.optional(p.space() + p.literal("<|constrain|>") + constrain_type);
1020+
auto constraint = p.optional(p.space() + p.optional(p.literal("<|constrain|>")) + constrain_type);
10171021
auto args = p.tool_args(p.schema(p.json(), "tool-" + name + "-schema", params));
10181022

10191023
// recipient in role header
@@ -1054,6 +1058,7 @@ static common_chat_params common_chat_params_init_gpt_oss(const common_chat_temp
10541058

10551059
data.grammar_triggers = {
10561060
{ COMMON_GRAMMAR_TRIGGER_TYPE_PATTERN, "^\\s+to$" },
1061+
{ COMMON_GRAMMAR_TRIGGER_TYPE_PATTERN, "^<\\|channel\\|>(?:commentary|analysis)\\s+to=functions$" },
10571062
{ COMMON_GRAMMAR_TRIGGER_TYPE_PATTERN, "<\\|start\\|>assistant(\\s+to)" },
10581063
{ COMMON_GRAMMAR_TRIGGER_TYPE_PATTERN, "<\\|start\\|>assistant(<\\|channel\\|>(?:commentary|analysis)\\s+to)" }
10591064
};

‎common/peg-parser.cpp‎

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1557,6 +1557,36 @@ static std::unordered_set<std::string> collect_reachable_rules(
15571557

15581558
// GBNF generation implementation
15591559
void common_peg_arena::build_grammar(const common_grammar_builder & builder, bool lazy) const {
1560+
auto schema_delegates = [](const common_peg_schema_parser & s) -> bool {
1561+
if (!s.schema) {
1562+
return true;
1563+
}
1564+
if (s.raw && s.schema->contains("type") && s.schema->at("type").is_string() && s.schema->at("type") == "string") {
1565+
return true;
1566+
}
1567+
return false;
1568+
};
1569+
1570+
// Unwrap the parser so we can properly check if it's a sequence or choice
1571+
auto effective_parser = [&](common_peg_parser_id id) -> const common_peg_parser_variant & {
1572+
while (true) {
1573+
const auto & p = parsers_.at(id);
1574+
if (const auto * tag = std::get_if<common_peg_tag_parser>(&p)) {
1575+
id = tag->child;
1576+
} else if (const auto * atomic = std::get_if<common_peg_atomic_parser>(&p)) {
1577+
id = atomic->child;
1578+
} else if (const auto * schema = std::get_if<common_peg_schema_parser>(&p)) {
1579+
if (schema_delegates(*schema)) {
1580+
id = schema->child;
1581+
} else {
1582+
return p;
1583+
}
1584+
} else {
1585+
return p;
1586+
}
1587+
}
1588+
};
1589+
15601590
// Generate GBNF for a parser
15611591
std::function<std::string(common_peg_parser_id)> to_gbnf = [&](common_peg_parser_id id) -> std::string {
15621592
const auto & parser = parsers_.at(id);
@@ -1577,7 +1607,7 @@ void common_peg_arena::build_grammar(const common_grammar_builder & builder, boo
15771607
s += " ";
15781608
}
15791609
auto child_gbnf = to_gbnf(child);
1580-
const auto & child_parser = parsers_.at(child);
1610+
const auto & child_parser = effective_parser(child);
15811611
if (std::holds_alternative<common_peg_choice_parser>(child_parser) ||
15821612
std::holds_alternative<common_peg_sequence_parser>(child_parser)) {
15831613
s += "(" + child_gbnf + ")";
@@ -1593,7 +1623,7 @@ void common_peg_arena::build_grammar(const common_grammar_builder & builder, boo
15931623
s += " | ";
15941624
}
15951625
auto child_gbnf = to_gbnf(child);
1596-
const auto & child_parser = parsers_.at(child);
1626+
const auto & child_parser = effective_parser(child);
15971627
if (std::holds_alternative<common_peg_choice_parser>(child_parser)) {
15981628
s += "(" + child_gbnf + ")";
15991629
} else {
@@ -1603,7 +1633,7 @@ void common_peg_arena::build_grammar(const common_grammar_builder & builder, boo
16031633
return s;
16041634
} else if constexpr (std::is_same_v<T, common_peg_repetition_parser>) {
16051635
auto child_gbnf = to_gbnf(p.child);
1606-
const auto & child_parser = parsers_.at(p.child);
1636+
const auto & child_parser = effective_parser(p.child);
16071637
if (std::holds_alternative<common_peg_choice_parser>(child_parser) ||
16081638
std::holds_alternative<common_peg_sequence_parser>(child_parser)) {
16091639
child_gbnf = "(" + child_gbnf + ")";
@@ -1663,15 +1693,10 @@ void common_peg_arena::build_grammar(const common_grammar_builder & builder, boo
16631693
}
16641694
return gbnf_excluding_pattern(p.delimiters);
16651695
} else if constexpr (std::is_same_v<T, common_peg_schema_parser>) {
1666-
if (p.schema) {
1667-
if (p.raw && p.schema->contains("type") && p.schema->at("type").is_string() && p.schema->at("type") == "string") {
1668-
// TODO: Implement more comprehensive grammar generation for raw strings.
1669-
// For now, use the grammar emitted from the underlying parser.
1670-
return to_gbnf(p.child);
1671-
}
1672-
return builder.add_schema(p.name, *p.schema);
1696+
if (schema_delegates(p)) {
1697+
return to_gbnf(p.child);
16731698
}
1674-
return to_gbnf(p.child);
1699+
return builder.add_schema(p.name, *p.schema);
16751700
} else if constexpr (std::is_same_v<T, common_peg_rule_parser>) {
16761701
return p.name;
16771702
} else if constexpr (std::is_same_v<T, common_peg_ref_parser>) {

‎tests/peg-parser/test-gbnf-generation.cpp‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,66 @@ void test_gbnf_generation(testing &t) {
213213
)""", gbnf);
214214
});
215215

216+
t.test("tagged choice inside sequence gets parenthesized", [](testing &t) {
217+
auto parser = build_peg_parser([](common_peg_parser_builder & p) {
218+
return p.literal("a") + p.tag("t", p.literal("b") | p.literal("c"));
219+
});
220+
221+
auto gbnf = build_grammar([&](const common_grammar_builder & builder) {
222+
parser.build_grammar(builder);
223+
});
224+
225+
assert_gbnf_equal(t, R"""(
226+
root ::= "a" ("b" | "c")
227+
space ::= | " " | "\n"{1,2} [ \t]{0,20}
228+
)""", gbnf);
229+
});
230+
231+
t.test("tagged sequence inside choice gets parenthesized", [](testing &t) {
232+
auto parser = build_peg_parser([](common_peg_parser_builder & p) {
233+
return p.tag("t", p.literal("a") + p.literal("b")) | p.literal("c");
234+
});
235+
236+
auto gbnf = build_grammar([&](const common_grammar_builder & builder) {
237+
parser.build_grammar(builder);
238+
});
239+
240+
assert_gbnf_equal(t, R"""(
241+
root ::= "a" "b" | "c"
242+
space ::= | " " | "\n"{1,2} [ \t]{0,20}
243+
)""", gbnf);
244+
});
245+
246+
t.test("atomic choice inside repetition gets parenthesized", [](testing &t) {
247+
auto parser = build_peg_parser([](common_peg_parser_builder & p) {
248+
return p.one_or_more(p.atomic(p.literal("a") | p.literal("b")));
249+
});
250+
251+
auto gbnf = build_grammar([&](const common_grammar_builder & builder) {
252+
parser.build_grammar(builder);
253+
});
254+
255+
assert_gbnf_equal(t, R"""(
256+
root ::= ("a" | "b")+
257+
space ::= | " " | "\n"{1,2} [ \t]{0,20}
258+
)""", gbnf);
259+
});
260+
261+
t.test("nested transparent wrappers get parenthesized", [](testing &t) {
262+
auto parser = build_peg_parser([](common_peg_parser_builder & p) {
263+
return p.literal("x") + p.tag("outer", p.atomic(p.literal("a") | p.literal("b")));
264+
});
265+
266+
auto gbnf = build_grammar([&](const common_grammar_builder & builder) {
267+
parser.build_grammar(builder);
268+
});
269+
270+
assert_gbnf_equal(t, R"""(
271+
root ::= "x" ("a" | "b")
272+
space ::= | " " | "\n"{1,2} [ \t]{0,20}
273+
)""", gbnf);
274+
});
275+
216276
t.test("emit only trigger rules (and references)", [](testing &t) {
217277
auto parser = build_peg_parser([](common_peg_parser_builder & p) {
218278
auto rule1 = p.rule("rule-1", p.literal("a") + p.ref("rule-2"));

‎tests/test-chat.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3175,6 +3175,24 @@ static void test_template_output_peg_parsers(bool detailed_debug) {
31753175
.expect_reasoning("I will execute python to say hello")
31763176
.expect_content("")
31773177
.run();
3178+
3179+
// Edge cases
3180+
3181+
// "<|channel|>commentary to=assistant" before reasoning
3182+
tst.test(
3183+
"<|channel|>commentary to=assistant<|channel|>analysis<|message|>I'm\nthinking<|end|><|start|>assistant<|channel|>final<|message|>Hello, world!\nWhat's "
3184+
"up?")
3185+
.reasoning_format(COMMON_REASONING_FORMAT_AUTO)
3186+
.expect(message_assist_thoughts)
3187+
.run();
3188+
3189+
// "<|channel|>commentary to=assistant" before final message
3190+
tst.test(
3191+
"<|channel|>analysis<|message|>I'm\nthinking<|end|><|start|>assistant<|channel|>commentary to=assistant<|channel|>final<|message|>Hello, world!\nWhat's "
3192+
"up?")
3193+
.reasoning_format(COMMON_REASONING_FORMAT_AUTO)
3194+
.expect(message_assist_thoughts)
3195+
.run();
31783196
}
31793197

31803198
{

0 commit comments

Comments
 (0)