Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
$defs ordering remains nondeterministic, and request serialization coverage should be added.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This pull request stabilizes generated schema and Ollama request serialization to improve prompt-prefix cache reuse.
Changes:
- Sorts schema properties and required fields.
- Uses sorted JSON keys for streaming and non-streaming Ollama requests.
| File | Summary |
|---|---|
Sources/AnyLanguageModel/Models/OllamaLanguageModel.swift |
Stabilizes request encoding; add serialized-body tests for insertion-order independence. |
Sources/AnyLanguageModel/GenerationSchema.swift |
Stabilizes schema ordering; $defs keys remain unsorted and should be canonicalized. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+526
to
+531
| private func encodeChatParams(_ params: [String: JSONValue]) throws -> Data { | ||
| let encoder = JSONEncoder() | ||
| // Ollama reuses prompt prefixes only when their serialized bytes match. | ||
| // Dictionary iteration order must not vary between equivalent requests. | ||
| encoder.outputFormatting = [.sortedKeys] | ||
| return try encoder.encode(params) |
Collaborator
|
Hi @akbashev. Thank you for this! It's a great catch, and the numbers from Ollama's logs make the case. Copilot pointed out two things to add before we merge:
If you'd rather not, just say so, and I'm happy to add these on top of your change. |
Author
|
@mattt good catch with $def, missed it 🤔🙂 Will do soon! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Something I've encountered in my project:
Ollama can reuse a cached prompt prefix when the next request begins with the same serialised content. Changes in JSON key or array order can break that match, even when the schemas mean the same thing.
AnyLanguageModel stores required property names in a Set. Encoding that set directly can produce different required array orders across requests. Sorting property names and required values makes generated schemas deterministic. Sorting JSON object keys in the Ollama request encoder keeps the final request serialization stable for both streaming and non-streaming calls.
Observed in Ollama’s logs:
These are observations from separate runs, not a controlled benchmark. They show the intended effect: equivalent schema content now produces a much more reusable prompt prefix.