fix: default when tools missing and minijinja length handling (#3880)

Signed-off-by: Ryan Lempka <rlempka@nvidia.com>
Signed-off-by: ryan-lempka <rlempka@nvidia.com>
Co-authored-by: Ryan McCormick <rmccormick@nvidia.com>

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);

lib/llm/src/preprocessor/prompt/template/oai.rs

@@ -145,11 +145,7 @@ impl OAIChatLikeRequest for NvCreateChatCompletionRequest {
 
     fn tools(&self) -> Option<Value> {
         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::<serde_json::Value>::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::<Value>::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() {