diff --git a/src/DotLLM.Tokenizers/ChatTemplates/JinjaParser.cs b/src/DotLLM.Tokenizers/ChatTemplates/JinjaParser.cs index 4e308ad3..07b1c6a8 100644 --- a/src/DotLLM.Tokenizers/ChatTemplates/JinjaParser.cs +++ b/src/DotLLM.Tokenizers/ChatTemplates/JinjaParser.cs @@ -591,10 +591,41 @@ private IExpression ParsePrimary() case JinjaTokenType.LeftParen: { + // Grouping `(expr)` vs. tuple literal `(a, b, ...)` / `(a,)` / `()`. + // + // Jinja2/Python semantics: a comma inside the parens — including a lone + // trailing comma after a single item — makes this a tuple; a bare `(expr)` + // with no comma is plain grouping and evaluates to `expr` itself. Tuples are + // represented with the same node used for `[...]` + // literals since the evaluator treats both as ordered sequences. Advance(); - var expr = ParseExpression(); + + if (CurrentIs(JinjaTokenType.RightParen)) + { + // `()` — empty tuple. + Advance(); + return new ListExpr([]); + } + + var first = ParseExpression(); + + if (CurrentIs(JinjaTokenType.Comma)) + { + var items = new List { first }; + while (CurrentIs(JinjaTokenType.Comma)) + { + Advance(); + if (CurrentIs(JinjaTokenType.RightParen)) + break; // trailing comma + items.Add(ParseExpression()); + } + + Expect(JinjaTokenType.RightParen); + return new ListExpr(items); + } + Expect(JinjaTokenType.RightParen); - return expr; + return first; } case JinjaTokenType.LeftBracket: diff --git a/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/Fixtures/qwen3.8-27b-chat-template.jinja b/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/Fixtures/qwen3.8-27b-chat-template.jinja new file mode 100644 index 00000000..c0c686f9 --- /dev/null +++ b/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/Fixtures/qwen3.8-27b-chat-template.jinja @@ -0,0 +1,170 @@ +{%- set image_count = namespace(value=0) %} +{%- set video_count = namespace(value=0) %} +{%- macro render_content(content, do_vision_count, is_system_content=false) %} + {%- if content is string %} + {{- content }} + {%- elif content is iterable and content is not mapping %} + {%- for item in content %} + {%- if 'image' in item or 'image_url' in item or item.type == 'image' %} + {%- if is_system_content %} + {{- raise_exception('System message cannot contain images.') }} + {%- endif %} + {%- if do_vision_count %} + {%- set image_count.value = image_count.value + 1 %} + {%- endif %} + {%- if add_vision_id %} + {{- 'Picture ' ~ image_count.value ~ ': ' }} + {%- endif %} + {{- '<|vision_start|><|image_pad|><|vision_end|>' }} + {%- elif 'video' in item or item.type == 'video' %} + {%- if is_system_content %} + {{- raise_exception('System message cannot contain videos.') }} + {%- endif %} + {%- if do_vision_count %} + {%- set video_count.value = video_count.value + 1 %} + {%- endif %} + {%- if add_vision_id %} + {{- 'Video ' ~ video_count.value ~ ': ' }} + {%- endif %} + {{- '<|vision_start|><|video_pad|><|vision_end|>' }} + {%- elif 'text' in item %} + {{- item.text }} + {%- else %} + {{- raise_exception('Unexpected item type in content.') }} + {%- endif %} + {%- endfor %} + {%- elif content is none or content is undefined %} + {{- '' }} + {%- else %} + {{- raise_exception('Unexpected content type.') }} + {%- endif %} +{%- endmacro %} +{%- if not messages %} + {{- raise_exception('No messages provided.') }} +{%- endif %} +{%- set reasoning_instructions = '' %} +{%- if enable_thinking is undefined or enable_thinking is true %} + {%- set resolved_reasoning_effort = reasoning_effort|default('xhigh') %} + {%- if resolved_reasoning_effort not in ('xhigh', 'medium', 'low') %} + {{- raise_exception('Unexpected reasoning effort ' ~ reasoning_effort ~ '. Supported types are xhigh (default), medium, and low.') }} + {%- endif %} + {%- if resolved_reasoning_effort == 'xhigh' %} + {%- set reasoning_instructions = 'Reasoning effort is set to xhigh. Please think carefully through the task, validate key assumptions, consider plausible alternatives, and prioritize correctness, consistency, and clarity in the final answer.' %} + {%- elif resolved_reasoning_effort == 'low' %} + {%- set reasoning_instructions = 'Reasoning effort is set to low. Keep your thinking brief and focused, moving directly to the conclusion without unnecessary elaboration.' %} + {%- endif %} +{%- endif %} +{%- if tools and tools is iterable and tools is not mapping %} + {{- '<|im_start|>system\n' }} + {%- if reasoning_instructions %} + {{- reasoning_instructions + '\n\n' }} + {%- endif %} + {{- "# Tools\n\nYou have access to the following functions:\n\n" }} + {%- for tool in tools %} + {{- "\n" }} + {{- tool | tojson }} + {%- endfor %} + {{- "\n" }} + {{- '\n\nIf you choose to call a function ONLY reply in the following format with NO suffix:\n\n\n\n\nvalue_1\n\n\nThis is the value for the second parameter\nthat can span\nmultiple lines\n\n\n\n\n\nReminder:\n- Function calls MUST follow the specified format: an inner block must be nested within XML tags\n- Required parameters MUST be specified\n- You may provide optional reasoning for your function call in natural language BEFORE the function call, but NOT after\n- If there is no function call available, answer the question like normal with your current knowledge and do not tell the user about function calls\n' }} + {%- if messages[0].role == 'system' %} + {%- set content = render_content(messages[0].content, false, true)|trim %} + {%- if content %} + {{- '\n\n' + content }} + {%- endif %} + {%- endif %} + {{- '<|im_end|>\n' }} +{%- else %} + {%- if messages[0].role == 'system' %} + {%- set content = render_content(messages[0].content, false, true)|trim %} + {%- if content %} + {{- '<|im_start|>system\n' + (reasoning_instructions + '\n\n' if reasoning_instructions else '') + content + '<|im_end|>\n' }} + {%- elif reasoning_instructions %} + {{- '<|im_start|>system\n' + reasoning_instructions + '<|im_end|>\n' }} + {%- endif %} + {%- elif reasoning_instructions %} + {{- '<|im_start|>system\n' + reasoning_instructions + '<|im_end|>\n' }} + {%- endif %} +{%- endif %} +{%- set ns = namespace(multi_step_tool=true, last_query_index=messages|length - 1) %} +{%- for message in messages[::-1] %} + {%- set index = (messages|length - 1) - loop.index0 %} + {%- if ns.multi_step_tool and message.role == "user" %} + {%- set content = render_content(message.content, false)|trim %} + {%- if not(content.startswith('') and content.endswith('')) %} + {%- set ns.multi_step_tool = false %} + {%- set ns.last_query_index = index %} + {%- endif %} + {%- endif %} +{%- endfor %} +{%- if ns.multi_step_tool %} + {{- raise_exception('No user query found in messages.') }} +{%- endif %} +{%- for message in messages %} + {%- set content = render_content(message.content, true)|trim %} + {%- if message.role == "system" %} + {%- if not loop.first %} + {{- raise_exception('System message must be at the beginning.') }} + {%- endif %} + {%- elif message.role == "user" %} + {{- '<|im_start|>' + message.role + '\n' + content + '<|im_end|>' + '\n' }} + {%- elif message.role == "assistant" %} + {%- set reasoning_content = '' %} + {%- if message.reasoning_content is string %} + {%- set reasoning_content = message.reasoning_content %} + {%- endif %} + {%- set reasoning_content = reasoning_content|trim %} + {%- if preserve_thinking is undefined or preserve_thinking is true or loop.index0 > ns.last_query_index %} + {{- '<|im_start|>' + message.role + '\n\n' + reasoning_content + '\n\n\n' + content }} + {%- else %} + {{- '<|im_start|>' + message.role + '\n' + content }} + {%- endif %} + {%- if message.tool_calls and message.tool_calls is iterable and message.tool_calls is not mapping %} + {%- for tool_call in message.tool_calls %} + {%- if tool_call.function is defined %} + {%- set tool_call = tool_call.function %} + {%- endif %} + {%- if loop.first %} + {%- if content|trim %} + {{- '\n\n\n\n' }} + {%- else %} + {{- '\n\n' }} + {%- endif %} + {%- else %} + {{- '\n\n\n' }} + {%- endif %} + {%- if tool_call.arguments is defined and tool_call.arguments != '' %} + {%- for args_name, args_value in tool_call.arguments|items %} + {{- '\n' }} + {%- set args_value = args_value | string if args_value is string else args_value | tojson | safe %} + {{- args_value }} + {{- '\n\n' }} + {%- endfor %} + {%- endif %} + {{- '\n' }} + {%- endfor %} + {%- endif %} + {{- '<|im_end|>\n' }} + {%- elif message.role == "tool" %} + {%- if loop.previtem and loop.previtem.role != "tool" %} + {{- '<|im_start|>user' }} + {%- endif %} + {{- '\n\n' }} + {{- content }} + {{- '\n' }} + {%- if not loop.last and loop.nextitem.role != "tool" %} + {{- '<|im_end|>\n' }} + {%- elif loop.last %} + {{- '<|im_end|>\n' }} + {%- endif %} + {%- else %} + {{- raise_exception('Unexpected message role.') }} + {%- endif %} +{%- endfor %} +{%- if add_generation_prompt %} + {{- '<|im_start|>assistant\n' }} + {%- if enable_thinking is defined and enable_thinking is false %} + {{- '\n\n\n\n' }} + {%- else %} + {{- '\n' }} + {%- endif %} +{%- endif %} \ No newline at end of file diff --git a/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/JinjaParserTests.cs b/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/JinjaParserTests.cs index 2c92255b..a6c9f7fa 100644 --- a/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/JinjaParserTests.cs +++ b/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/JinjaParserTests.cs @@ -321,4 +321,110 @@ public void FilterWithArgs() Assert.Equal("default", filter.FilterName); Assert.Single(filter.Args); } + + // ── Tuple literals (#409) ── + // + // Jinja2/Python semantics: parentheses containing a comma-separated expression list + // form a tuple; a lone comma after a single item ("(a,)") is significant and still makes + // a tuple, while a comma-free "(a)" is plain grouping and must NOT become a tuple. + + [Fact] + public void ParenGrouping_SingleExpression_NoTrailingComma_IsNotATuple() + { + // "(a)" must parse identically to "a" — no ListExpr wrapper. This is the case a + // naive "any parens with a comma-splittable body" implementation gets wrong. + var grouped = Parse("{{ (x) }}"); + var groupedOutput = Assert.IsType(grouped.Nodes[0]); + Assert.IsType(groupedOutput.Expression); + + var plain = Parse("{{ x }}"); + var plainOutput = Assert.IsType(plain.Nodes[0]); + Assert.Equal( + ((IdentifierExpr)plainOutput.Expression).Name, + ((IdentifierExpr)groupedOutput.Expression).Name); + } + + [Fact] + public void ParenGrouping_ArithmeticPrecedence_StillWorks() + { + // "(a)" grouping must still compose normally with surrounding operators. + var ast = Parse("{{ (1 + 2) * 3 }}"); + var output = Assert.IsType(ast.Nodes[0]); + var binary = Assert.IsType(output.Expression); + Assert.Equal(BinaryOp.Multiply, binary.Op); + Assert.IsType(binary.Left); // the "(1 + 2)" grouping, unwrapped + } + + [Fact] + public void TupleLiteral_OneElementWithTrailingComma_IsAOneElementTuple() + { + // "(a,)" — the trailing comma is significant: distinct from plain "(a)" grouping. + var ast = Parse("{{ (x,) }}"); + var output = Assert.IsType(ast.Nodes[0]); + var tuple = Assert.IsType(output.Expression); + Assert.Single(tuple.Items); + Assert.IsType(tuple.Items[0]); + } + + [Fact] + public void TupleLiteral_MultipleElements() + { + var ast = Parse("{{ ('xhigh', 'medium', 'low') }}"); + var output = Assert.IsType(ast.Nodes[0]); + var tuple = Assert.IsType(output.Expression); + Assert.Equal(3, tuple.Items.Count); + Assert.Equal("xhigh", ((LiteralExpr)tuple.Items[0]).Value); + Assert.Equal("medium", ((LiteralExpr)tuple.Items[1]).Value); + Assert.Equal("low", ((LiteralExpr)tuple.Items[2]).Value); + } + + [Fact] + public void TupleLiteral_TrailingCommaAfterMultipleElements_IsLegal() + { + var ast = Parse("{{ (1, 2, 3,) }}"); + var output = Assert.IsType(ast.Nodes[0]); + var tuple = Assert.IsType(output.Expression); + Assert.Equal(3, tuple.Items.Count); + } + + [Fact] + public void TupleLiteral_Empty() + { + var ast = Parse("{{ () }}"); + var output = Assert.IsType(ast.Nodes[0]); + var tuple = Assert.IsType(output.Expression); + Assert.Empty(tuple.Items); + } + + [Fact] + public void TupleLiteral_NotIn_TrueBranch() + { + // The literal construct from Qwen3.8-27B's chat_template.jinja line 48. + var ast = Parse( + "{%- if resolved_reasoning_effort not in ('xhigh', 'medium', 'low') -%}yes{%- endif -%}"); + var ifNode = Assert.IsType(ast.Nodes[0]); + var condition = ifNode.Branches[0].Condition; + var unary = Assert.IsType(condition); + Assert.Equal(UnaryOp.Not, unary.Op); + var binary = Assert.IsType(unary.Operand); + Assert.Equal(BinaryOp.In, binary.Op); + var tuple = Assert.IsType(binary.Right); + Assert.Equal(3, tuple.Items.Count); + } + + [Fact] + public void TupleLiteral_In_EvaluatesTrueWhenMember() + { + var ast = Parse("{%- if effort in ('xhigh', 'medium', 'low') -%}matched{%- else -%}unmatched{%- endif -%}"); + var evaluator = new JinjaEvaluator(new Dictionary { ["effort"] = "medium" }); + Assert.Equal("matched", evaluator.Evaluate(ast)); + } + + [Fact] + public void TupleLiteral_NotIn_EvaluatesTrueWhenNotMember() + { + var ast = Parse("{%- if effort not in ('xhigh', 'medium', 'low') -%}matched{%- else -%}unmatched{%- endif -%}"); + var evaluator = new JinjaEvaluator(new Dictionary { ["effort"] = "auto" }); + Assert.Equal("matched", evaluator.Evaluate(ast)); + } } diff --git a/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/JinjaQwen3_8_27BAcceptanceTests.cs b/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/JinjaQwen3_8_27BAcceptanceTests.cs new file mode 100644 index 00000000..1e9aa0de --- /dev/null +++ b/tests/DotLLM.Tests.Unit/Tokenizers/ChatTemplates/JinjaQwen3_8_27BAcceptanceTests.cs @@ -0,0 +1,89 @@ +using System.Runtime.CompilerServices; +using DotLLM.Tokenizers; +using DotLLM.Tokenizers.ChatTemplates; +using Xunit; + +namespace DotLLM.Tests.Unit.Tokenizers.ChatTemplates; + +/// +/// End-to-end acceptance test for issue #409: the real, unmodified +/// Qwen/Qwen3.8-27B chat_template.jinja must at least PARSE successfully once the +/// tuple-literal grouping bug is fixed. The fixture is byte-for-byte what HuggingFace serves at +/// https://huggingface.co/Qwen/Qwen3.8-27B/raw/main/chat_template.jinja (fetched 2026-08-14). +/// +public class JinjaQwen3_8_27BAcceptanceTests +{ + private static string LoadFixture([CallerFilePath] string callerFilePath = "") + { + var dir = Path.GetDirectoryName(callerFilePath)!; + var path = Path.Combine(dir, "Fixtures", "qwen3.8-27b-chat-template.jinja"); + return File.ReadAllText(path); + } + + [Fact] + public void FullTemplate_Parses_WithoutThrowing() + { + // This is the exact regression from #409: before the tuple-literal fix, this throws + // "Line 48, Col 53: Expected RightParen, got Comma" on + // `resolved_reasoning_effort not in ('xhigh', 'medium', 'low')`, which sits in the + // reasoning-instructions prelude that executes unconditionally for every render. + var source = LoadFixture(); + var tokens = new JinjaLexer(source).Tokenize(); + var parser = new JinjaParser(tokens); + var ast = parser.Parse(); + Assert.NotEmpty(ast.Nodes); + } + + [Fact] + public void FullTemplate_ConstructsAsJinjaChatTemplate_WithoutThrowing() + { + // JinjaChatTemplate's constructor lexes+parses eagerly; this is the same assertion as + // above but through the public API callers actually use. + var source = LoadFixture(); + _ = new JinjaChatTemplate(source, bosToken: "<|endoftext|>", eosToken: "<|im_end|>"); + } + + [Fact] + public void FullTemplate_Render_KnownRemainingGapIsTracked() + { + // Rendering (as opposed to parsing) additionally requires #399's loop.previtem / + // loop.nextitem support and `is undefined` handling — both used unconditionally in this + // template (the reasoning-effort prelude at line 46: `enable_thinking is undefined or ...`, + // and the tool-response run detection at lines 148/154: `loop.previtem` / `loop.nextitem`). + // #399 is tracked separately by PR #411, which is NOT merged as of this branch. + // + // This test documents the current end-to-end state rather than silently skipping: + // - If #411 has NOT landed: rendering must fail, and it must fail for the KNOWN #399 + // reason (an unsupported `is undefined`/`is defined`-style test name), not for a + // tuple/comma parsing reason — proving #409's fix is not masking a different bug. + // - If #411 HAS landed (merge this branch onto a newer `dev` and re-run): rendering + // should succeed outright; this test's `catch` branch will no longer be reached and + // the success path below is asserted instead. + var source = LoadFixture(); + var template = new JinjaChatTemplate(source, bosToken: "<|endoftext|>", eosToken: "<|im_end|>"); + + var messages = new[] + { + new ChatMessage { Role = "user", Content = "What is 2 + 2?" }, + }; + var options = new ChatTemplateOptions { AddGenerationPrompt = true }; + + try + { + var result = template.Apply(messages, options); + + // #411 has landed (or the gap has otherwise closed) — full end-to-end render works. + Assert.Contains("What is 2 + 2?", result); + } + catch (JinjaException ex) + { + // Must NOT be a tuple/grouping parse failure (that would mean #409 regressed). + Assert.DoesNotContain("RightParen", ex.Message); + Assert.DoesNotContain("got Comma", ex.Message); + + // Must be the known, tracked #399 gap: an "is undefined"-style test name the + // evaluator doesn't recognize yet (see JinjaEvaluator.EvalIsTest). + Assert.Contains("Unknown test", ex.Message); + } + } +}