diff --git a/lib/llm/src/preprocessor/prompt/template/formatters.rs b/lib/llm/src/preprocessor/prompt/template/formatters.rs index 4579ffa41..82a63a6bf 100644 --- a/lib/llm/src/preprocessor/prompt/template/formatters.rs +++ b/lib/llm/src/preprocessor/prompt/template/formatters.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use super::tokcfg::{ChatTemplate, raise_exception, strftime_now, tojson}; use super::{ContextMixins, HfTokenizerConfigJsonFormatter, JinjaEnvironment}; use either::Either; -use minijinja::Environment; +use minijinja::{Environment, Value}; use tracing; impl JinjaEnvironment { @@ -34,6 +34,17 @@ impl HfTokenizerConfigJsonFormatter { "chat_template field is required in the tokenizer_config.json file" ))?; + // Safely handle chat templates that check the length of arguments like `tools` even + // when `tools=None` when rendered through minijinja. For example: + // https://github.com/vllm-project/vllm/blob/d95d0f4b985f28ea381e301490f9d479b34d8980/examples/tool_chat_template_hermes.jinja#L36 + env.add_filter("length", |value: Value| -> usize { + use minijinja::value::ValueKind; + match value.kind() { + ValueKind::Undefined | ValueKind::None => 0, + _ => value.len().unwrap_or(0), + } + }); + // add pycompat // todo: should we use this: minijinja_contrib::add_to_environment(&mut env); env.set_unknown_method_callback(minijinja_contrib::pycompat::unknown_method_callback); diff --git a/lib/llm/src/preprocessor/prompt/template/oai.rs b/lib/llm/src/preprocessor/prompt/template/oai.rs index aa0d37367..23428e13d 100644 --- a/lib/llm/src/preprocessor/prompt/template/oai.rs +++ b/lib/llm/src/preprocessor/prompt/template/oai.rs @@ -145,11 +145,7 @@ impl OAIChatLikeRequest for NvCreateChatCompletionRequest { fn tools(&self) -> Option { if self.inner.tools.is_none() { - // ISSUE: {%- if tools is iterable and tools | length > 0 %} - // For cases like above, minijinja will not error out in calculating the length of tools - // as it evaluates both the sides an don't do short circuiting. - // Safe to return an empty array here. This will work even if tools are not present as length = 0 - Some(Value::from_serialize(Vec::::new())) + None } else { // Try to fix the tool schema if it is missing type and properties Some(may_be_fix_tool_schema( @@ -590,6 +586,71 @@ mod tests { assert_eq!(content_array[1]["type"], "image_url"); } + #[test] + fn test_none_tools_safe_for_all_templates() { + use super::tokcfg::ChatTemplate; + use super::{ContextMixins, HfTokenizerConfigJsonFormatter}; + + // Due to minijinja limitations the expressions in conditional statements may not be short-circuited + // This checks that our custom length filter works to avoid errors in this scenario + // length should return 0 if tools is None and 'if tools is iterable and tools | length > 0' should evaluate to false + let length_template = r#" +{%- if tools is iterable and tools | length > 0 %} +Tools available: {{ tools | length }} +{%- else %} +No tools +{%- endif %} +"#; + + // Because we return None for tools when there are no tools this scenario should also be evaluate to false + // This is similar to the default jinja template behavior seen with llama models which check if tools is not none to activate tool mode + let no_tool_template = r#" +{%- if tools is not none %} +TOOL MODE +{%- else %} +NORMAL MODE +{%- endif %} +"#; + + let chat_template: ChatTemplate = serde_json::from_value(serde_json::json!({ + "chat_template": [ + {"safe_length": length_template}, + {"no_tool": no_tool_template} + ] + })) + .unwrap(); + + let formatter = + HfTokenizerConfigJsonFormatter::new(chat_template, ContextMixins::new(&[])).unwrap(); + + let ctx = context! { tools => Option::::None }; + + let result1 = formatter + .env + .get_template("safe_length") + .unwrap() + .render(&ctx); + println!("Safe length template with no tools => None: {:?}", result1); + assert!( + result1.is_ok(), + "Jinja template with and conditional and length filter should handle None: {:?}", + result1 + ); + assert!( + result1.unwrap().contains("No tools"), + "Should show 'No tools'" + ); + + let result2 = formatter.env.get_template("no_tool").unwrap().render(&ctx); + println!("Default template with no tools => None: {:?}", result2); + assert!( + result2.is_ok(), + "Jinja template with if tools is not none conditional should handle None: {:?}", + result2 + ); + assert!(result2.unwrap().contains("NORMAL MODE")); + } + /// Tests mixed content type scenarios. #[test] fn test_may_be_fix_msg_content_multiple_content_types() {