fix(A1): iOS vision 'Multimodal support not enabled' — keep only precision in mmproj on-disk name#605
Open
alichherawalla wants to merge 7 commits into
Open
fix(A1): iOS vision 'Multimodal support not enabled' — keep only precision in mmproj on-disk name#605alichherawalla wants to merge 7 commits into
alichherawalla wants to merge 7 commits into
Conversation
…HF vision repos Hits real Hugging Face (no mocks) for every curated vision repo + popular families, runs the real pair→rename→belongs pipeline, and asserts the projector belongs after rename. Red today for the ggml-org `mmproj-<ModelName>-<quant>` shape (SmolVLM, Qwen2.5-VL, pixtral, InternVL). Also locks the separate no-pairing bug set as a tracked ledger.
ggml-org names projectors `mmproj-<ModelName>-<quant>.gguf` (model name AFTER the mmproj token). The old `/mmproj.*$/` kept that model name, doubling the on-disk identity stem, so mmProjBelongsToModel refused it, linkOrphanMmProj cleared the link, and the model loaded text-only → initMultimodal never ran → 'Multimodal support not enabled' on a vision send (device 2026-07-23, iPhone 12, SmolVLM). Extract only the precision token so the on-disk stem always matches the model. Verified against live HF: 8 rename-broken repos now pass, the 6 already-working stay working.
…ne test Replace the always-network tests (flaky, slow) with the capture-once/reuse pattern: UPDATE_HF_FIXTURES=1 downloads the real HF listing + real GGUF header bytes for every vision repo into a committed fixture; normal runs replay the real pair→rename→belongs pipeline over that captured real data in ~0.5s, offline, deterministic. Reverting the fix flips the ggml-org curated repos red. Tracks the separate no-pairing bug as a ledger.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughVision projector matching now normalizes role and packaging markers, recognizes generic projector placeholders, and derives precision-based local names. Unit tests and committed Hugging Face fixtures validate pairing, renamed ownership, and GGUF probes across vision repositories. ChangesVision projector pairing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant visionMmprojFromHFTest
participant HuggingFaceAPI
participant mmproj
participant FixtureFile
visionMmprojFromHFTest->>HuggingFaceAPI: Fetch repository GGUF listing
HuggingFaceAPI-->>visionMmprojFromHFTest: Return model and projector filenames
visionMmprojFromHFTest->>mmproj: Select paired projector and derive local name
visionMmprojFromHFTest->>HuggingFaceAPI: Probe selected files with range requests
HuggingFaceAPI-->>visionMmprojFromHFTest: Return GGUF probe metadata
visionMmprojFromHFTest->>FixtureFile: Persist or load fixture data
visionMmprojFromHFTest->>mmproj: Validate pairing and renamed ownership
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/integration/models/visionMmprojFromHF.test.ts`:
- Around line 55-61: Update fetchTreeGgufFiles to throw an error when the
Hugging Face fetch response is not OK instead of returning an empty array.
Preserve the existing parsing and filtering behavior for successful responses so
fixture refreshes cannot persist transient failures as missing files.
- Around line 145-156: Update the noPairing filter in the known-open repository
test to include repositories with missing pairs, failed
repoPairsAndBelongs(repo).belongs ownership, or an invalid probe result. Keep
the existing tracked repository expectation unchanged so any non-known-open
pairing or rename regression, including SmolVLM, fails the test.
In `@src/services/modelManager/download.ts`:
- Around line 35-36: Update the precision extraction in the projector naming
logic near mmProjSourceName so it selects the final precision-like token, or
specifically the token associated with the .gguf suffix, rather than the first
match. Preserve the existing fallback to mmproj.gguf, and add a regression test
covering mmproj-Q8_0-Vision-f16.gguf expecting mmproj-f16.gguf.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f6e6c115-115f-440b-bb69-f6f01b9ddf32
📒 Files selected for processing (4)
__tests__/fixtures/hf/vision-repos.json__tests__/integration/models/visionMmprojFromHF.test.ts__tests__/unit/services/parallelMmproj.test.tssrc/services/modelManager/download.ts
…eam) Fixes the second mmproj bug class found in the live-HF sweep, so ALL vision-projector shapes pair + belong: - `mmproj-model-<prec>` literal placeholder → treated as a generic repo-single-model projector (ggml-org gemma-3, openbmb MiniCPM). - unsloth `UD-` dynamic-quant packaging stripped from the identity stem so a UD-quant model matches its non-UD projector (Mistral-Small-3.1). - moondream `-text-model` weights marker normalized so the weights and `-mmproj` file share a stem. Strict matching preserved — E2B still refuses an E4B projector (unit test). Verified over live-HF fixture data: every curated + popular vision repo now pairs and its on-disk name belongs.
…oken) Addresses SonarCloud (super-linear regex backtracking, download.ts:35) and CodeRabbit (take the projector's OWN trailing precision, not the first precision-like token in the name) in one change. Also: fixture refresh now throws on a HF 429/5xx instead of silently persisting an empty fixture.
|
…d, root-cause pending device log
…Android fail was transient (load-timing watch-item, not a blocker)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/GAPS_BACKLOG.md`:
- Around line 377-379: Update the new documentation entry around “A1 mmproj fix”
by removing both em dashes, replacing them with a colon or parentheses while
preserving the heading and verdict meaning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+377
to
+379
| ## A1 mmproj fix — possible load-immediately-after-download vision race (2026-07-24) | ||
|
|
||
| **Verdict: instrument-and-revisit (LOW — unconfirmed). NOT a merge blocker; PR #605 verified on both |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove em dashes from the new documentation entry.
Replace the em dashes in the heading and verdict with a colon or parentheses.
As per coding guidelines, documentation content must avoid em dashes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/GAPS_BACKLOG.md` around lines 377 - 379, Update the new documentation
entry around “A1 mmproj fix” by removing both em dashes, replacing them with a
colon or parentheses while preserving the heading and verdict meaning.
Source: Coding guidelines
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
1 participant
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.
Fixes ALL vision-projector (mmproj) bugs on device — the iOS "Multimodal support not enabled" crash and the family of projector-naming mismatches behind it.
Bug (device report 2026-07-23, Christophe — iPhone 12, SmolVLM-256M)
A ggml-org vision model loads, Repair reports success, image attaches, then generation throws "Multimodal support not enabled. Call initMultimodal first." unsloth Qwen worked — a publisher-specific split, not a family one.
Two seams, both fixed
1. On-disk rename (
mmProjLocalName) — ggml-org names projectorsmmproj-<ModelName>-<quant>.gguf(model name AFTERmmproj). The old/mmproj.*$/baked that name in → doubled identity stem →mmProjBelongsToModelrefused it →linkOrphanMmProjcleared the link → text-only load →initMultimodalnever ran. Fix: keep only the precision token → always<model>-mmproj-<precision>.gguf.2. Download pairing (
pickMmProjForDownload/modelIdentityStem) — a second class found in the live-HF sweep where no projector paired at all:mmproj-model-<prec>literal placeholder (ggml-org gemma-3, openbmb MiniCPM) → treated as a generic repo-single-model projector.UD-dynamic-quant packaging (Mistral-Small-3.1) → stripped from the identity stem.-text-modelweights marker → normalized so weights and-mmprojshare a stem.Strict matching preserved — E2B still refuses an E4B projector (no wrong-arch mispairing).
Verification
visionMmprojFromHF.test.ts): captures the REAL Hugging Face listing + real GGUF download probes for every curated + popular vision repo, replays the real pair→rename→belongs pipeline offline. Now asserts EVERY vision repo (all naming shapes) pairs and its on-disk name belongs. Reverting either fix turns repos red. Plus offline unit regressions inparallelMmproj.test.ts.smolvlm-256m-instruct-mmproj-Q8_0.gguf(fixed name) and[WIRE-VISION] … initialized:true, support:{vision:true}.Summary by CodeRabbit
Bug Fixes
Tests
Documentation