ollama: Don't override model's default stop tokens (#48119)

little Kitchen created 3 weeks ago

## Summary

When no stop tokens are provided, Zed was sending an empty array
(`"stop": []`) to Ollama. This caused Ollama to override the model's
default stop tokens (defined in its Modelfile) with nothing, resulting
in models like rnj-1:8b generating infinitely with literal stop tokens
appearing in the output.

## Problem

Models with custom stop tokens in their Modelfile (like `<|eot_id|>` for
rnj-1:8b) would generate forever because:

1. Agent thread creates request with `stop: Vec::new()` (empty)
2. Ollama provider converts this to `stop: Some(vec![])`
3. Serializes as `"stop": []` in JSON
4. Ollama interprets this as "override default stop tokens with nothing"
5. Model generates forever, outputting stop tokens as literal text

## Solution

1. In `crates/language_models/src/provider/ollama.rs`:
   - Only send `stop` when explicitly provided
   - When empty, use `None` so the field is omitted from JSON
   
2. In `crates/ollama/src/ollama.rs`:
- Add `#[serde(skip_serializing_if = "Option::is_none")]` to all
`ChatOptions` fields
   - Ensures `None` values are omitted, not serialized as `null`

## Testing

Added 4 new tests in `crates/ollama/src/ollama.rs`:
- `test_chat_options_serialization`: Verifies None fields are omitted
- `test_chat_request_with_stop_tokens`: Verifies stop tokens are
serialized when provided
- `test_chat_request_without_stop_tokens_omits_field`: Verifies empty
stop is omitted

All 11 ollama tests pass, plus 1 language_models ollama test.

Fixes #47798

Release Notes:

- Fixed Ollama models with custom stop tokens generating infinitely by
not overriding model defaults when no stop tokens are specified.

Change summary

crates/language_models/src/provider/ollama.rs |  9 +
crates/ollama/src/ollama.rs                   | 97 +++++++++++++++++++++
2 files changed, 105 insertions(+), 1 deletion(-)

Detailed changes

crates/language_models/src/provider/ollama.rs

@@ -400,7 +400,14 @@ impl OllamaLanguageModel {
             stream: true,
             options: Some(ChatOptions {
                 num_ctx: Some(self.model.max_tokens),
-                stop: Some(request.stop),
+                // Only send stop tokens if explicitly provided. When empty/None,
+                // Ollama will use the model's default stop tokens from its Modelfile.
+                // Sending an empty array would override and disable the defaults.
+                stop: if request.stop.is_empty() {
+                    None
+                } else {
+                    Some(request.stop)
+                },
                 temperature: request.temperature.or(Some(1.0)),
                 ..Default::default()
             }),

crates/ollama/src/ollama.rs

@@ -123,10 +123,15 @@ pub struct ChatRequest {
 // https://github.com/ollama/ollama/blob/main/docs/modelfile.md#valid-parameters-and-values
 #[derive(Serialize, Default, Debug)]
 pub struct ChatOptions {
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub num_ctx: Option<u64>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub num_predict: Option<isize>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub stop: Option<Vec<String>>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub temperature: Option<f32>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub top_p: Option<f32>,
 }
 
@@ -588,4 +593,96 @@ mod tests {
         assert_eq!(message_images.len(), 1);
         assert_eq!(message_images[0].as_str().unwrap(), base64_image);
     }
+
+    #[test]
+    fn test_chat_options_serialization() {
+        // When stop is None, it should not appear in JSON at all
+        // This allows Ollama to use the model's default stop tokens
+        let options_no_stop = ChatOptions {
+            num_ctx: Some(4096),
+            stop: None,
+            temperature: Some(0.7),
+            ..Default::default()
+        };
+        let serialized = serde_json::to_string(&options_no_stop).unwrap();
+        assert!(
+            !serialized.contains("stop"),
+            "stop should not be in JSON when None"
+        );
+        assert!(serialized.contains("num_ctx"));
+        assert!(serialized.contains("temperature"));
+
+        // When stop has values, they should be serialized
+        let options_with_stop = ChatOptions {
+            stop: Some(vec!["<|eot_id|>".to_string()]),
+            ..Default::default()
+        };
+        let serialized = serde_json::to_string(&options_with_stop).unwrap();
+        assert!(serialized.contains("stop"));
+        assert!(serialized.contains("<|eot_id|>"));
+
+        // All None options should result in empty object
+        let options_all_none = ChatOptions::default();
+        let serialized = serde_json::to_string(&options_all_none).unwrap();
+        assert_eq!(serialized, "{}");
+    }
+
+    #[test]
+    fn test_chat_request_with_stop_tokens() {
+        let request = ChatRequest {
+            model: "rnj-1:8b".to_string(),
+            messages: vec![ChatMessage::User {
+                content: "Hello".to_string(),
+                images: None,
+            }],
+            stream: true,
+            keep_alive: KeepAlive::default(),
+            options: Some(ChatOptions {
+                stop: Some(vec!["<|eot_id|>".to_string(), "<|end|>".to_string()]),
+                ..Default::default()
+            }),
+            think: None,
+            tools: vec![],
+        };
+
+        let serialized = serde_json::to_string(&request).unwrap();
+        let parsed: serde_json::Value = serde_json::from_str(&serialized).unwrap();
+
+        let stop = parsed["options"]["stop"].as_array().unwrap();
+        assert_eq!(stop.len(), 2);
+        assert_eq!(stop[0].as_str().unwrap(), "<|eot_id|>");
+        assert_eq!(stop[1].as_str().unwrap(), "<|end|>");
+    }
+
+    #[test]
+    fn test_chat_request_without_stop_tokens_omits_field() {
+        // This tests the fix for issue #47798
+        // When no stop tokens are provided, the field should be omitted
+        // so Ollama uses the model's default stop tokens from Modelfile
+        let request = ChatRequest {
+            model: "rnj-1:8b".to_string(),
+            messages: vec![ChatMessage::User {
+                content: "Hello".to_string(),
+                images: None,
+            }],
+            stream: true,
+            keep_alive: KeepAlive::default(),
+            options: Some(ChatOptions {
+                num_ctx: Some(4096),
+                stop: None, // No stop tokens - should be omitted from JSON
+                ..Default::default()
+            }),
+            think: None,
+            tools: vec![],
+        };
+
+        let serialized = serde_json::to_string(&request).unwrap();
+
+        // The key check: "stop" should not appear in the serialized JSON
+        assert!(
+            !serialized.contains("\"stop\""),
+            "stop field should be omitted when None, got: {}",
+            serialized
+        );
+    }
 }