feat: allow user import model include mmproj file #6388
Merged
Conversation
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
urmauur
requested review from
Minh141120,
dinhlongviolin1,
louis-jan and
qnixsynapse
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 1b035fd in 1 minute and 55 seconds. Click for details.
- Reviewed
693lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/ChatInput.tsx:107
- Draft comment:
Refactored mmproj check call using multiline chaining enhances readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/containers/DropdownModelProvider.tsx:194
- Draft comment:
Improved auto-selection logic using a currentState object helps avoid race conditions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/routes/settings/providers/$providerName.tsx:527
- Draft comment:
Refactored inline model import logic to use ImportVisionModelDialog, improving maintainability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_akhTy6MGV3Xx2nqj
You can customize 
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4141910 in 1 minute and 39 seconds. Click for details.
- Reviewed
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:42
- Draft comment:
Removing the file filters allows any file type to be selected. Consider re-adding filters (with correct extensions: 'gguf' for model and 'mmproj' for vision models) or adding client-side validation to prevent incorrect file types. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While removing filters does allow any file type to be selected initially, the application has multiple layers of validation: 1) Clear UI labeling of required file types 2) File extension validation when processing the file name 3) Backend validation during import. The filters were also incorrect before (using 'gguf' for both model and mmproj). Having filters might provide slightly better UX but isn't a critical issue. I might be underestimating the importance of UX - letting users select wrong files initially only to fail later could be frustrating. Also, I'm assuming the backend does proper validation. While better UX is valuable, this isn't a critical issue - the application handles invalid files gracefully and clearly communicates requirements. The previous filters were actually wrong, so removing them may be better than having incorrect ones. This comment should be deleted. While file filters could improve UX, the application has sufficient validation and clear communication about file requirements. The change doesn't introduce any bugs or critical issues.
Workflow ID: wflow_bWrIbKrTl6RvXL11
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 836990b in 1 minute and 22 seconds. Click for details.
- Reviewed
100lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:1738
- Draft comment:
checkMmprojExists now verifies mmproj existence by first checking for a YAML field and then falling back to fs.existsSync. Consider validating that a defined mmproj_path is non‐empty and optionally checking the file’s existence even when mmproj_path is provided. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:25
- Draft comment:
The onSuccess callback signature now accepts an optional importedModelName, and it is correctly called with modelName on success. This enables downstream components to update state based on the imported model. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/routes/settings/providers/$providerName.tsx:93
- Draft comment:
The handleModelImportSuccess function refreshes the provider settings and, for llamacpp, checks for mmproj existence to add the 'vision' capability to the model. Note that it assumes the imported model's id equals the model name passed from the dialog; ensure this mapping remains consistent. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_Fg4Qu6rEX9JQ59mF
You can customize
|
LGTM! Next actions:
|
qnixsynapse
approved these changes
Sep 8, 2025
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed be851eb in 2 minutes and 45 seconds. Click for details.
- Reviewed
596lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:2004
- Draft comment:
Ensure validateGgufFile documents its dual role clearly: for text generation models it rejects clip architecture, while for vision models (via mmproj) clip is expected. Adding an inline comment to clarify this difference would be beneficial. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:64
- Draft comment:
The validation logic for model vs. MMProj files is duplicated. Consider refactoring the common code into a helper to improve maintainability. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. web-app/src/services/models/default.ts:470
- Draft comment:
In the fallback of validateGgufFile, returning isValid:true along with an error message is potentially confusing. Consider omitting the error field when isValid is true or adjusting the fallback logic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid point about API design - having both isValid=true and an error message is somewhat contradictory and could confuse consumers of this API. However, this is a fallback case where we're deliberately being lenient by assuming validity when we can't validate. The error message serves as documentation of why we couldn't properly validate. The current implementation seems like a reasonable tradeoff. The comment may be too nitpicky about API design in a fallback case. The current implementation provides useful information while maintaining backward compatibility. While the API design could be cleaner, the current implementation serves a practical purpose by both allowing the file through (isValid=true) while documenting why proper validation couldn't occur. The comment should be deleted. The current implementation makes a reasonable tradeoff between usability and proper error documentation in a fallback case.
4. web-app/src/services/models/types.ts:72
- Draft comment:
The types and interfaces look clean. Ensure that any future changes to model validation also update the ModelValidationResult interface if additional fields are required. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
5. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:99
- Draft comment:
Typo suggestion: The error message for MMProj files refers to the expected architecture as "clip" (in lowercase) on this line, whereas the corresponding model file error message uses "CLIP". Consider standardizing the casing (e.g., using "CLIP" in both messages) for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the inconsistency exists, it's a very minor issue that doesn't affect functionality. The code correctly handles both cases since the comparison is done with lowercase. The error messages are still clear and understandable regardless of casing. This seems like an overly pedantic comment that doesn't warrant a code change. The inconsistency could potentially confuse users who are trying to debug issues, as they might think the casing matters when it actually doesn't. The potential for confusion is minimal since these are just error messages, and the actual validation logic uses lowercase comparison. Users are unlikely to even notice the casing difference. Delete this comment as it addresses a trivial cosmetic issue that doesn't impact functionality or clarity.
Workflow ID: wflow_HDpxHehGafX7XIx8
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f259413 in 1 minute and 40 seconds. Click for details.
- Reviewed
66lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:15
- Draft comment:
Remove unused import 'IconFileImport'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:430
- Draft comment:
Updated CSS classes for error messages to use standardized design tokens ('text-destructive' and 'text-destructive/90'). Verify that these changes match the intended design system. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:459
- Draft comment:
Changed file selection button variant to 'link' with updated styling classes. Ensure the new styling provides the expected visual affordance. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_dO4OZjXmgaNbnPSE
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 94dc298 in 1 minute and 58 seconds. Click for details.
- Reviewed
151lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:65
- Draft comment:
If serviceHub.models().validateGgufFile isn’t available for model files, no validation occurs. Consider logging a warning or handling this case explicitly. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:118
- Draft comment:
Before accessing nested metadata (metadata.metadata), consider verifying the structure to guard against undefined values. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:124
- Draft comment:
If 'architecture' is undefined, the error message may be confusing. Consider checking for undefined before using it in the error message. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:60
- Draft comment:
Multiple console.log statements output file paths and metadata. In production, consider a more secure logging approach or removal of debug logs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:106
- Draft comment:
Using a dynamic import for Tauri's invoke is useful; ensure this pattern is supported in all target environments and errors are handled gracefully. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:47
- Draft comment:
Consider extracting the file validation logic into separate utility functions to improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_4aF0tAsrwG7Rw4Aa
You can customize
|
@urmauur, the MMPROJ file should be cleaned up when we disable vision model support, otherwise when we try to import a text-based model, we'll encounter this error
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a5b0ced in 1 minute and 8 seconds. Click for details.
- Reviewed
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:314
- Draft comment:
Good update: The onCheckedChange handler now resets mmProj-related state when toggling off vision mode, preventing stale state. Consider memoizing the handler if performance becomes a concern. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_96P7r7xsQkUK4Iwk
You can customize
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.
Describe Your Changes
This pull request introduces a new dialog for importing vision models, refactors the model import logic in the provider settings page to use this dialog, and improves model/provider selection logic to avoid race conditions. It also includes minor UI and code quality improvements.
New Feature: Vision Model Import Dialog
ImportVisionModelDialogcomponent that allows users to import both standard and vision (MMPROJ) models, with validation, user feedback, and automatic model naming. This dialog is now used in the provider settings page for model import. [1] [2]Provider Settings Refactoring
$providerName.tsx) to remove old inline model import logic and use the newImportVisionModelDialog, simplifying the code and improving maintainability. [1] [2]Model/Provider Selection Logic Improvements
DropdownModelProvider.tsxto avoid race conditions, especially when handling web-only auto-selection and manual selections. Excluded certain dependencies fromuseEffectto prevent unwanted re-renders. [1] [2] [3] [4] [5] [6]UI Consistency
Code Cleanup
Fixes Issues
Self Checklist
Important
Introduces
ImportVisionModelDialogfor importing models, refactors provider settings, and improves model/provider selection logic.ImportVisionModelDialogfor importing standard and vision models with validation and feedback.$providerName.tsxto useImportVisionModelDialogfor model import.DropdownModelProvider.tsxto avoid race conditions.useEffectto prevent re-renders.This description was created by
for a5b0ced. You can customize this summary. It will automatically update as commits are pushed.