Skip to content

Conversation

@atdrendel
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings December 6, 2025 14:38
Copy link

Copilot AI left a 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.

Comment on lines +201 to +202
#expect(reasoning1 != nil)
#expect(text1 == nil)
Copy link

Copilot AI Dec 6, 2025

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.

Suggested change
#expect(reasoning1 != nil)
#expect(text1 == nil)

Copilot uses AI. Check for mistakes.
self = .parsingToolCall("")
return nil

case (.parsingToolCall(var buffer), .chunk(endToken)):
Copy link

Copilot AI Dec 6, 2025

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.

Suggested change
case (.parsingToolCall(var buffer), .chunk(endToken)):
case (.parsingToolCall(var buffer), .chunk(endToken)):
assert(buffer.isEmpty)

Copilot uses AI. Check for mistakes.
@atdrendel atdrendel merged commit 20ba887 into main Dec 8, 2025
1 check passed
@atdrendel atdrendel deleted the fix-reasoning branch December 8, 2025 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants