fix: #3476 - Mismatch id between model json and path

[louis-menlo]

Describe Your Changes

This PR addresses issue #3476, where manually added models are broken due to a mismatched ID between the model.json file and the folder path, as the model_id in the JSON file does not match the folder name where the model is stored, causing issues with model recognition and functionality.

The first attempt in the past to address this is to make model_id the same as folder structure BUT that could lead to an issue where:

• Model ID should not be auto-generated in some cases (remote models).
• The folder structure is not the source of truth (nested / symlink).

After thinking through, one of the better fixes is to work with ModelFile (a type alias of Model & File), where:

1. It contains Model metadata, which is used widely in the app.
2. It contains File metadata, which is used for file path retrieval.
3. Remove the constraint of model ID and the JSON file path.

Deprecate Model Preserve Settings (will be replaced by Preset)
It's also removed the Preserve Model Settings, which isn't aligned well and might cause unexpected error (direct model.json overwrite)
https://discord.com/channels/1107178041848909847/1283644162838892564/1283749880937971774

Changes made (Generated)

Changes in Advanced component

• Removed preserveModelSettingsAtom import and related code.
• Removed the "Preserve Model Settings" section.

Changes in ModelDownloadRow component

• Renamed isDownloaded to downloadedModel.
• Updated requestCreateNewThread function to use downloadedModel instead of model.

Changes in MyModelList component

• Renamed model type to ModelFile.

Removed features

• restoreModelSettings functionality has been removed.

Refactoring

• Improved variable names and code organization.

Removed deprecated code

• Removed isDownloaded variable and replaced with downloadedModel.

Other changes

• Improved type definitions and code consistency.

Added test cases (Generated)

1. getDownloadedModels() with GGUF files:

   • Test case checks if getDownloadedModels() returns downloaded models with correct file paths and model IDs.
   • Test case checks if getDownloadedModels() returns downloaded models when there are GGUF files in the model folder.
   • Test case checks if getDownloadedModels() returns downloaded models when there are multiple GGUF files in the model folder (for nested folders).

2. getDownloadedModels() with uppercased GGUF files:

   • Test case checks if getDownloadedModels() returns downloaded models with correct file paths and model IDs.
   • Test case checks if getDownloadedModels() returns downloaded models when there are uppercased GGUF files in the model folder.

3. getDownloadedModels() with GGUF & TensorRT files:

   • Test case checks if getDownloadedModels() returns downloaded models with correct file paths and model IDs.
   • Test case checks if getDownloadedModels() returns downloaded models when there are both GGUF and TensorRT files in the model folder.

4. deleteModel() for GGUF models:

   • Test case checks if deleteModel() deletes the GGUF file when the model is a GGUF model.
   • Test case checks if deleteModel() does not delete any files when the GGUF file is not present.
   • Test case checks if deleteModel() deletes the entire model folder when the model is an imported model.
   • Test case checks if deleteModel() deletes the TensorRT file when the model is a TensorRT model.

5. (Implicit) Test case checks if deleteModel() calls fs.unlinkSync() for GGUF files and fs.rm() for TensorRT files and entire model folders for imported models.

Fixes Issues

• bug: Mismatch ID between model.json and folder path #3476

Self Checklist

• Added relevant comments, esp in complex areas
• Updated docs (for bug fixes / features)
• Created issues for follow-up changes or refactoring needed

[github-actions]

Barecheck - Code coverage report

Total: 57.49%

Your code coverage diff: 0.18% ▴

Uncovered files and lines

File	Lines
core/src/browser/core.ts	29, 113, 124, 131, 140
core/src/browser/extensions/model.ts	20
core/src/browser/extensions/engines/AIEngine.ts	22, 24-25, 32, 36, 38-41, 43-45, 48-50, 53, 56-57, 63-67, 71, 74, 82-84, 90-92
core/src/browser/extensions/engines/LocalOAIEngine.ts	13-14, 23, 25-26, 33-36, 46-48, 50-52, 59, 61-63
core/src/node/api/processors/app.ts	73, 77, 81, 88-89

[dan-menlo]

@louis-jan Can I verify my understanding of this PR, and our overall strategy for handling legacy and future Model Folder:

• This PR addresses the "legacy" Nitro-based Model Folder structure
• It is handled through import InferenceEngine from @janhq/core
• Can I verify that this is the inference-nitro-extension?

In other words, will this have any impact on #3633?

[louis-menlo]

@louis-jan Can I verify my understanding of this PR, and our overall strategy for handling legacy and future Model Folder:

• This PR addresses the "legacy" Nitro-based Model Folder structure
• It is handled through import InferenceEngine from @janhq/core
• Can I verify that this is the inference-nitro-extension?

In other words, will this have any impact on #3633?

Hi @dan-homebrew, great concerns.

1. This PR addresses the "legacy" Nitro-based Model Folder structure
   Yes, it addresses the legacy model folder. Now, the app works with a 1:1 map between model.json content and its path. It's the same effect as models.list but in-memory instead of persisted.

2. It is handled through

extensionManager
    .get<ModelExtension>(ExtensionTypeEnum.Model)
    ?.getDownloadedModels()

In other words, Model Extension, where it loads model DTOs from model folders.

It is handled from LocalOAIExtension, so any local engines will be affected.

In short, it's just a simple fix: Update the DTO, where the Model Extension previously delivered only the model.json content, now includes the original path.

[dan-menlo]

Got it - I'll focus my follow-up questions in this separate issue:
#3633

[louis-menlo]

Feature branch builds: https://github.com/janhq/jan/actions/runs/10871614291

[dan-menlo]

@louis-jan Can I give a quick check before we approve it for 0.5.5:

Bricking Risk + State Changes

• What is the "bricking" risk this might have on our userbase?
• My understanding is that there are no "state" changes
• The change is the we now point to a "ModelFile", i.e. model.json

Nitro vs. Cortex

• My naive understanding is that this covers Nitro legacy model folder structure
• However, I notice that we are changing the model extension, and DTOs
• Will this in any way affect the Cortex Model Folder and model.yaml (or will they be handled by a separate handler in the model extension/DTO?)

[louis-menlo]

@louis-jan Can I give a quick check before we approve it for 0.5.5:

Bricking Risk + State Changes

• What is the "bricking" risk this might have on our userbase?
• My understanding is that there are no "state" changes
• The change is the we now point to a "ModelFile", i.e. model.json

Nitro vs. Cortex

• My naive understanding is that this covers Nitro legacy model folder structure
• However, I notice that we are changing the model extension, and DTOs
• Will this in any way affect the Cortex Model Folder and model.yaml (or will they be handled by a separate handler in the model extension/DTO?)

Hi @dan-homebrew,

Bricking Risk + State Changes

1. Yes, the same effect (can't load model) occurs if the DTO gives a wrong path, but it is not persisted (this won't touch user model.json or folder structure) and can be patched.
2. Yes, there is no state change, but the DTO.

Nitro vs. Cortex

1. The issue is not about the Nitro legacy model structure, but rather about the Model Folder & model.json (both local and remote).
2. The DTO should not affect the persistence objects. Since there should be many DTOs which present an object. Furthermore, the app could not depend on a specific local engine yet. For example, remote models or other engines that could not work with C++ or have to port. But still can be delivered as an extension.

[dan-menlo]

@louis-jan Can I give a quick check before we approve it for 0.5.5:
Bricking Risk + State Changes

• What is the "bricking" risk this might have on our userbase?
• My understanding is that there are no "state" changes
• The change is the we now point to a "ModelFile", i.e. model.json

Nitro vs. Cortex

• My naive understanding is that this covers Nitro legacy model folder structure
• However, I notice that we are changing the model extension, and DTOs
• Will this in any way affect the Cortex Model Folder and model.yaml (or will they be handled by a separate handler in the model extension/DTO?)

Hi @dan-homebrew,

Bricking Risk + State Changes

1. Yes, the same effect (can't load model) occurs if the DTO gives a wrong path, but it is not persisted (this won't touch user model.json or folder structure) and can be patched.
2. Yes, there is no state change, but the DTO.

Nitro vs. Cortex

1. The issue is not about the Nitro legacy model structure, but rather about the Model Folder & model.json (both local and remote).
2. The DTO should not affect the persistence objects. Since there should be many DTOs which present an object. Furthermore, the app could not depend on a specific local engine yet. For example, remote models or other engines that could not work with C++ or have to port. But still can be delivered as an extension.

Thanks @louis-jan. Let's go ahead to merge, and use the v0.5.5 QA process to catch issues.

[louis-menlo]

Thanks @dan-homebrew. I will wait for the release cut before I merge this.

[dan-menlo]

This will need to be QA-ed rigorously before we release in 0.5.5

[louis-menlo]

@dan-homebrew. You're right, I've tested and found an edge case with local model download, an older scenario that we should still account for - I'll take a closer look before the release.