janhq / jan

Jan is an open source alternative to ChatGPT that runs 100% offline on your computer. Multiple engine support (llama.cpp, TensorRT-LLM)
https://jan.ai/
GNU Affero General Public License v3.0
23.1k stars 1.34k forks source link

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

Closed imtuyethan closed 1 month ago

imtuyethan commented 2 months ago

Current behavior

Manually added models are broken due to a mismatched ID between the model.json file and the folder path. The model_id in the JSON file doesn't match the folder name where the model is stored, causing issues with model recognition and functionality.

Minimum reproduction step

  1. Manually add a model to Jan
  2. Create a model.json file with an "id" that doesn't match the folder name
  3. Place the model.json file in a folder with a different name
  4. Attempt to use the manually added model in Jan

Expected behavior

Jan should consistently recognize and use manually added models, regardless of minor discrepancies between the model.json "id" field and the folder name. Ideally, Jan should use the folder name as the authoritative model_id to ensure consistency.

Screenshots / Logs

Screenshot 2024-08-27 at 9 40 07 PM

Jan version

-

In which operating systems have you tested?

Environment details

-

louis-jan commented 2 months ago

There is an edge case where a customized OpenAI-compatible remote model could have a / in its model ID. So auto generated ID would not work for the case unless users create nested folder to correct this. (Which is not a good workaround)

louis-jan commented 2 months ago

Ref: https://discord.com/channels/1107178041848909847/1110389363809976361/1278650955214229534

dan-homebrew commented 2 months ago

@louis-jan @marknguyen1302 I think Model Folder detection and maintenance is going to important for Jan + Cortex to get right. We currently have 3 main model sources:

Nitro

Cortex

What is our current logic that we currently use for Model detection, in the Model Folder? We should be really careful - I probably screwed up asking for folder names to be part of the design, and this will break in all sorts of ways cross-OS.

dan-homebrew commented 2 months ago

@louis-jan @marknguyen1302 Additionally, to prepare for our upcoming Cortex migration, can we write test coverage for our existing Model Folder (i.e. all the variants we have, local, remote, etc).

I think we need to make sure we don't break for existing users, and whatever patchwork of assumptions, edge cases are accounted for, before we make changes.

We will likely need to do extensive Upgrade Testing, i.e. to make sure we don't break existing functionality for users.

Jan is pretty much a single feature tool now: i.e. download models to our folder structure and run them. Investing in test coverage for this critical functionality will give us a stable base to build off in the longer term

dan-homebrew commented 1 month ago

@louis-jan @imtuyethan @urmauur Quick check: will we do a hotfix for this in v0.5.4?

louis-jan commented 1 month ago

Yeah, we should right? There still a long way to do a full cortex.cpp migration. I can take this.

louis-jan commented 1 month ago

After investigation, both the model's ID generation and retrieving a folder from the model ID would not work and pose a high risk of breaking.

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

There is no need to constrain the model ID and the JSON file path