-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: prevent duplicate LM Studio models by only showing loaded models #7141
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
- Remove listDownloadedModels() call and its try/catch block - Only fetch loaded models as they are the only ones that can process requests - Use model.path as consistent key to prevent duplicates - Update tests to reflect the simplified implementation This fixes the issue where models appeared twice in the Provider Configuration Profile when they were both downloaded and loaded in LM Studio. Fixes #6954
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| models[lmstudioModel.modelKey] = parseLMStudioModel(lmstudioModel) | ||
| modelsWithLoadedDetails.add(lmstudioModel.modelKey) | ||
| // Use model.path as the consistent key to prevent duplicates | ||
| models[lmstudioModel.path] = parseLMStudioModel(lmstudioModel) |
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.
Good simplification! Using model.path as the consistent key should effectively prevent duplicates. However, I notice that the forceFullModelDetailsLoad function (not in this diff) still uses modelId to add to modelsWithLoadedDetails. Should that be updated to use the model path for consistency?
| console.warn("Failed to list downloaded models, falling back to loaded models only") | ||
| } | ||
| // We want to list loaded models *anyway* since they provide valuable extra info (context size) | ||
| // Only get loaded models - these are the only ones that can actually process requests |
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.
Nice change to only fetch loaded models. This makes sense since only loaded models can actually process requests. The comment clearly explains the rationale.
|
|
||
| const expectedParsedModel = parseLMStudioModel(mockRawModel) | ||
| expect(result).toEqual({ [mockRawModel.modelKey]: expectedParsedModel }) | ||
| // Now using model.path as the key instead of modelKey |
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.
Good test update! The assertion correctly verifies that we're now using model.path as the key instead of modelKey.
| }) | ||
|
|
||
| it("should fall back to listLoaded when listDownloadedModels fails", async () => { | ||
| it("should fetch only loaded models and use model.path as key", async () => { |
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.
Excellent test name update that clearly describes the new behavior - fetching only loaded models and using model.path as the key.
|
Not a good feature to have: LM Studio, when configured with Just-in-Time (JIT) Model Loading, can automatically load an unloaded model upon receiving an API call that requests that specific model. Since 2024 october this change is effective. https://lmstudio.ai/blog/lmstudio-v0.3.5#on-demand-model-loading Also there is a auto model unload on 60 minutes timeout. So, it is better to show all models with simple deduplication. |
|
Closing this PR based on feedback from @mechanicmuthu about LM Studio's JIT Model Loading feature (available since v0.3.5). The current approach of only showing loaded models doesn't align well with the JIT loading capability where models can be automatically loaded on-demand. Will open a new PR with a simpler deduplication approach that shows all models while preventing duplicates. |
Summary
This PR fixes the issue where LM Studio models appeared twice in the Provider Configuration Profile when they were both downloaded and loaded.
Problem
The previous implementation fetched models from two different LM Studio APIs:
listDownloadedModels()- showed all models on disklistLoaded()- showed models currently in memoryThis caused duplicates when a model was both downloaded and loaded, with inconsistent keys (
model.pathvsmodel.modelKey).Solution
As suggested by @daniel-lxs, this PR simplifies the implementation by:
listDownloadedModels()call entirely (lines 74-83)model.pathas the consistent key to prevent any potential duplicatesBenefits
UX Change
Users will need to load models in LM Studio before they appear in Roo Code. This is already required to use them anyway, so it aligns the UI with the actual functionality.
Testing
listDownloadedModelsfunctionalitymodel.pathas the keyFixes #6954
Important
Fixes duplicate LM Studio models by removing
listDownloadedModels()and usingmodel.pathas the key inlmstudio.ts.listDownloadedModels()call inlmstudio.ts, only usinglistLoaded()to fetch models.model.pathas the key to prevent duplicates.lmstudio.test.tsto reflect removal oflistDownloadedModels().model.pathas the key.This description was created by
for 6f7c4bc. You can customize this summary. It will automatically update as commits are pushed.