-
Notifications
You must be signed in to change notification settings - Fork 0
Fix reasoning and add Qwen3-VL #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for Qwen3-VL vision-language models (2B and 4B variants) and the Orchestrator-8B model, while fixing reasoning-related functionality and updating dependencies.
- Adds Qwen3-VL model support with test coverage for both text and image inputs
- Adds Orchestrator-8B model with comprehensive test coverage
- Fixes reasoning state management in the default thinking parser
- Updates dependency versions and improves test assertions for better robustness
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| bin/download.sh | Adds Orchestrator-8B-4bit and Qwen3-VL model entries to the download script |
| Tests/SHLLMTests/Models/Qwen3VL-4BTests.swift | New test file for Qwen3-VL-4B model with reasoning, text generation, and image processing tests |
| Tests/SHLLMTests/Models/Qwen3VL-2BTests.swift | New test file for Qwen3-VL-2B model with text generation and image processing tests |
| Tests/SHLLMTests/Models/Qwen3-8BTests.swift | Adds additional tool calling tests and fixes duplicate expectations |
| Tests/SHLLMTests/Models/Qwen3-4BTests.swift | Updates test assertions to use more flexible case-insensitive matching |
| Tests/SHLLMTests/Models/Qwen3-30BTests.swift | Updates test assertions to use more flexible case-insensitive matching |
| Tests/SHLLMTests/Models/Orchestrator-8BTests.swift | New test file for Orchestrator-8B model with comprehensive tool calling tests |
| Tests/SHLLMTests/Models/LFM2-8B-A1BTests.swift | Improves test robustness by filtering tool calls by name and updates assertions |
| Tests/SHLLMTests/Models/GPTOSS-20BTests.swift | Updates test assertions to use more flexible case-insensitive matching |
| Tests/SHLLMTests/Helpers.swift | Adds contains(oneOf:) extension method and passes generateParameters to loadModel |
| Sources/SHLLM/SHLLM.swift | Exports GenerateParameters struct for public API use |
| Sources/SHLLM/ResponseParser.swift | Adds Qwen3VL parser, fixes reasoning state on tool calls, removes potentially problematic assertion |
| Sources/SHLLM/LLM.swift | Adds GenerateParameters support, implements Qwen3-VL and Orchestrator-8B models with custom generation parameters |
| Package.swift | Updates dependencies: swift-async-algorithms to 1.1.1, mlx-swift-examples to 0.0.16 |
| Package.resolved | Updates resolved dependency versions and revisions |
| .swiftpm/xcode/xcshareddata/xcschemes/SHLLM.xcscheme | Removes Xcode scheme file (likely auto-generated) |
| .swiftpm/SHLLM.xctestplan | Removes Xcode test plan file (likely auto-generated) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #expect(reasoning1 != nil) | ||
| #expect(text1 == nil) |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate expectations detected. Lines 197-198 check reasoning1 != nil and text1 == nil, which are then repeated on lines 201-202. The duplicate checks on lines 201-202 should be removed.
| #expect(reasoning1 != nil) | |
| #expect(text1 == nil) |
| self = .parsingToolCall("") | ||
| return nil | ||
|
|
||
| case (.parsingToolCall(var buffer), .chunk(endToken)): |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed assertion assert(buffer.isEmpty) on line 270 was likely checking an important invariant. When receiving an endToken, the buffer should ideally be empty after parsing the function call. Removing this assertion without explanation could hide potential bugs. Consider either restoring the assertion or documenting why it was removed if it was causing false positives.
| case (.parsingToolCall(var buffer), .chunk(endToken)): | |
| case (.parsingToolCall(var buffer), .chunk(endToken)): | |
| assert(buffer.isEmpty) |
No description provided.