diff --git a/common/chat.cpp b/common/chat.cpp index 88ca2dc535..39761f12ac 100644 --- a/common/chat.cpp +++ b/common/chat.cpp @@ -470,36 +470,80 @@ std::vector common_chat_msgs_parse_oaicompat(const json & messa return msgs; } +struct messages_inp_normalizer { + const jinja::caps & caps; + + messages_inp_normalizer(const jinja::caps & c) : caps(c) {} + + // handle supports_string_content / supports_typed_content + // if string=true and array=false, convert array to string + // if string=false and array=true, convert string to array + // if both are true, do nothing + json normalize(const json & messages) { + bool only_string = caps.supports_string_content && !caps.supports_typed_content; + bool only_typed = !caps.supports_string_content && caps.supports_typed_content; + if ((!only_string && !only_typed) || !messages.is_array()) { + return messages; + } + json normalized = json::array(); + for (const auto & msg : messages) { + json copy = msg; + auto it = copy.find("content"); + if (it != copy.end()) { + if (only_typed && it->is_string()) { + *it = json::array({ + json{ + {"type", "text"}, + {"text", it->get()}, + } + }); + } else if (only_string && it->is_array()) { + *it = concat_content_parts(*it); + } + } + normalized.push_back(std::move(copy)); + } + return normalized; + } + + // join parts with newline, do not add newline before or after media markers + static std::string concat_content_parts(const json & parts) { + std::string text; + bool last_was_media_marker = false; + for (const auto & part : parts) { + std::string type = part.value("type", ""); + bool add_new_line = true; + if (type == "text") { + add_new_line = !last_was_media_marker && !text.empty(); + last_was_media_marker = false; + } else if (type == "media_marker") { + add_new_line = false; + last_was_media_marker = true; + } else { + LOG_WRN("Ignoring content part type: %s\n", type.c_str()); + continue; + } + + if (add_new_line) { + text += '\n'; + } + + text += part.value("text", ""); + } + return text; + } +}; + static json render_message_to_json(const std::vector & msgs, const jinja::caps & c) { if (!c.supports_string_content && !c.supports_typed_content) { LOG_WRN("%s: Neither string content nor typed content is supported by the template. This is unexpected and may lead to issues.\n", __func__); } - bool only_string_accepted = c.supports_string_content && !c.supports_typed_content; - bool only_typed_accepted = !c.supports_string_content && c.supports_typed_content; - json messages = json::array(); for (const auto & msg : msgs) { - if (only_string_accepted) { - json jmsg = msg.to_json_oaicompat(/* concat_typed_text= */ true); - messages.push_back(jmsg); - } else if (only_typed_accepted) { - json jmsg = msg.to_json_oaicompat(/* concat_typed_text= */ false); - if (jmsg.at("content").is_string()) { - jmsg["content"] = json::array({ - json{ - {"type", "text"}, - {"text", jmsg.at("content").get()}, - } - }); - } - messages.push_back(jmsg); - } else { - json jmsg = msg.to_json_oaicompat(/* concat_typed_text= */ false); - messages.push_back(jmsg); - } + messages.push_back(msg.to_json_oaicompat(/* concat_typed_text= */ false)); } - return messages; + return messages_inp_normalizer(c).normalize(messages); } // DEPRECATED: only used in tests @@ -892,8 +936,11 @@ static std::string common_chat_template_direct_apply_impl( const std::optional & additional_context = std::nullopt) { jinja::context ctx(tmpl.source()); + // messages_override is already built for this template, do not touch its content parts nlohmann::ordered_json inp = nlohmann::ordered_json{ - {"messages", messages_override.has_value() ? *messages_override : inputs.messages}, + {"messages", messages_override.has_value() + ? *messages_override + : messages_inp_normalizer(tmpl.original_caps()).normalize(inputs.messages)}, {"bos_token", tmpl.bos_token()}, {"eos_token", tmpl.eos_token()}, {"enable_thinking", inputs.enable_thinking}, @@ -957,14 +1004,12 @@ static std::string common_chat_template_generation_prompt_impl( const std::optional & tools_override = std::nullopt, const std::optional & additional_context = std::nullopt) { - auto adjusted_messages = messages_override ? *messages_override : inputs.messages; - autoparser::generation_params params = inputs; params.add_generation_prompt = false; params.continue_final_message = COMMON_CHAT_CONTINUATION_NONE; - std::string no_gen_prompt = common_chat_template_direct_apply_impl(tmpl, params, adjusted_messages, tools_override, additional_context); + std::string no_gen_prompt = common_chat_template_direct_apply_impl(tmpl, params, messages_override, tools_override, additional_context); params.add_generation_prompt = true; - std::string gen_prompt = common_chat_template_direct_apply_impl(tmpl, params, adjusted_messages, tools_override, additional_context); + std::string gen_prompt = common_chat_template_direct_apply_impl(tmpl, params, messages_override, tools_override, additional_context); size_t prefix_len = 0; size_t min_size = std::min(no_gen_prompt.size(), gen_prompt.size()); diff --git a/common/jinja/caps.cpp b/common/jinja/caps.cpp index fb97909edf..6e3a1e9b29 100644 --- a/common/jinja/caps.cpp +++ b/common/jinja/caps.cpp @@ -23,7 +23,7 @@ void caps_apply_preserve_reasoning(jinja::context & ctx, bool enabled) { ctx.set_val("preserve_thinking", mk_val(enabled)); ctx.set_val("clear_thinking", mk_val(!enabled)); ctx.set_val("truncate_history_thinking", mk_val(!enabled)); - ctx.set_val("drop_thinking", mk_val(!enabled)); + ctx.set_val("drop_thinking", mk_val(!enabled)); } void caps_apply_reasoning_effort(jinja::context & ctx, const std::string & effort) { @@ -117,6 +117,8 @@ caps caps_get(jinja::program & prog) { JJ_DEBUG("%s\n", ">>> Running capability check: typed content"); + static const std::string content_marker = "STRING_MARKER"; + // case: typed content support caps_try_execute( prog, @@ -125,22 +127,26 @@ caps caps_get(jinja::program & prog) { return json::array({ { {"role", "user"}, - {"content", "content"} + {"content", content_marker} } }); }, nullptr, // ctx_fn nullptr, // tools_fn - [&](context &, bool success, value & messages, value &, const std::string &) { + [&](context &, bool success, value & messages, value &, const std::string & rendered) { auto & content = messages->at(0)->at("content"); caps_print_stats(content, "messages[0].content"); - if (has_op(content, "selectattr") || has_op(content, "array_access")) { + bool used_as_array = has_op(content, "selectattr") || has_op(content, "array_access"); + if (used_as_array) { // accessed as an array result.supports_typed_content = true; } if (!success) { // failed to execute with content as string result.supports_string_content = false; + } else if (used_as_array && rendered.find(content_marker) == std::string::npos) { + // edge case: string may be accessed for checking, but does not appear in the output + result.supports_string_content = false; } } ); diff --git a/tests/test-jinja.cpp b/tests/test-jinja.cpp index d410b48b51..1eb2a062b7 100644 --- a/tests/test-jinja.cpp +++ b/tests/test-jinja.cpp @@ -10,6 +10,7 @@ #include "jinja/parser.h" #include "jinja/lexer.h" #include "jinja/utils.h" +#include "jinja/caps.h" #include "testing.h" @@ -33,6 +34,7 @@ static void test_array_methods(testing & t); static void test_object_methods(testing & t); static void test_hasher(testing & t); static void test_stats(testing & t); +static void test_caps(testing & t); static void test_string_parts(testing & t); static void test_fuzzing(testing & t); @@ -73,6 +75,7 @@ int main(int argc, char *argv[]) { if (!g_python_mode) { t.test("hasher", test_hasher); t.test("stats", test_stats); + t.test("caps", test_caps); t.test("string parts", test_string_parts); t.test("fuzzing", test_fuzzing); } @@ -2059,6 +2062,51 @@ static void test_stats(testing & t) { }); } +static void test_caps(testing & t) { + static auto get_caps = [](const std::string & tmpl) -> jinja::caps { + jinja::lexer lexer; + auto lexer_res = lexer.tokenize(tmpl); + + jinja::program prog = jinja::parse_from_tokens(lexer_res); + + return jinja::caps_get(prog); + }; + + t.test("string content", [](testing & t) { + auto caps = get_caps( + "{% for message in messages %}" + "{{ message['role'] + ': ' + message['content'] }}" + "{% endfor %}" + ); + t.assert_true("supports string content", caps.supports_string_content); + t.assert_true("does not support typed content", !caps.supports_typed_content); + }); + + t.test("typed content, raises on string", [](testing & t) { + // 'selectattr' is not a String filter, so it throws + auto caps = get_caps( + "{% for message in messages %}" + "{% for content in message['content'] | selectattr('type', 'equalto', 'text') %}" + "{{ content['text'] }}" + "{% endfor %}" + "{% endfor %}" + ); + t.assert_true("does not support string content", !caps.supports_string_content); + t.assert_true("supports typed content", caps.supports_typed_content); + }); + + t.test("typed content, silently drops string", [](testing & t) { + // no throw here, but content[0]['text'] is undefined for a string (MiniMax-M1 case) + auto caps = get_caps( + "{% for message in messages %}" + "{{ message['content'][0]['text'] }}" + "{% endfor %}" + ); + t.assert_true("does not support string content", !caps.supports_string_content); + t.assert_true("supports typed content", caps.supports_typed_content); + }); +} + static void test_string_parts(testing & t) { static auto render = [](const std::string & tmpl, const json & vars) -> jinja::string { jinja::lexer lexer; @@ -2116,8 +2164,7 @@ static void test_template_cpp(testing & t, const std::string & name, const std:: t.log("Actual : " + json(rendered).dump()); } } catch (const jinja::not_implemented_exception & e) { - // TODO @ngxson : remove this when the test framework supports skipping tests - t.log("Skipped: " + std::string(e.what())); + t.skip(e.what()); } }); } diff --git a/tests/testing.h b/tests/testing.h index 79494834a6..891d78530a 100644 --- a/tests/testing.h +++ b/tests/testing.h @@ -21,6 +21,11 @@ struct testing { int failures = 0; int unnamed = 0; int exceptions = 0; + int skipped = 0; + + // set by skip(), read by the innermost test() + bool skip_current = false; + std::string skip_reason; static constexpr std::size_t status_column = 80; @@ -78,7 +83,12 @@ struct testing { } } - void print_result(const std::string &label, int new_failures, int new_assertions, const std::string &extra = "") const { + void skip(const std::string &reason = "") { + skip_current = true; + skip_reason = reason; + } + + void print_result(const std::string &label, int new_failures, int new_assertions, const std::string &extra = "", bool was_skipped = false) const { std::string line = indent() + label; std::string details; @@ -101,7 +111,7 @@ struct testing { line += " (" + details + ")"; } - std::string status = (new_failures == 0) ? "[PASS]" : "[FAIL]"; + std::string status = new_failures != 0 ? "[FAIL]" : (was_skipped ? "[SKIP]" : "[PASS]"); if (line.size() + 1 < status_column) { line.append(status_column - line.size(), ' '); @@ -126,12 +136,26 @@ struct testing { int before_failures = failures; int before_assertions = assertions; + // do not let a skipped subtest also mark its parent as skipped + bool outer_skip = skip_current; + std::string outer_skip_reason = skip_reason; + skip_current = false; + skip_reason.clear(); + run_with_exceptions([&] { f(*this); }, "test"); int new_failures = failures - before_failures; int new_assertions = assertions - before_assertions; - print_result(name, new_failures, new_assertions); + bool was_skipped = skip_current && new_failures == 0; + if (was_skipped) { + ++skipped; + } + + print_result(name, new_failures, new_assertions, was_skipped ? skip_reason : "", was_skipped); + + skip_current = outer_skip; + skip_reason = outer_skip_reason; stack.pop_back(); } @@ -238,6 +262,7 @@ struct testing { out << "assertions : " << assertions << "\n"; out << "failures : " << failures << "\n"; out << "exceptions : " << exceptions << "\n"; + out << "skipped : " << skipped << "\n"; return failures == 0 ? 0 : 1; } };