Skip to content
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

Fixing Ollama (and other dynamicModel providers) (re: #259) #344

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ssmirr
Copy link

@ssmirr ssmirr commented Nov 20, 2024

fixes #259.

the main bug effecting #259 is that MODEL_LIST did not include dynamicModels (for providers that support it). In fact dynamicModels were not fetched at all, meaning this is not only breaking Ollama provider, but also any other provider + model that uses dynamicModels.

that resulted in falling back to the DEFAULT_MODEL (i.e. claude-3-5-sonnet-latest) being selected as the currentModel because this condition always failed to find requested model:
https://github.com/coleam00/bolt.new-any-llm/blob/88700c24526bb5d722d48e51d0f2bb1cc77fb72a/app/lib/.server/llm/stream-text.ts#L51-L60

this PR constructs a list of dynamicModels by fetching it from all providers in parallel, and fixes MODEL_LIST to include those models as well.

let me know if you have any questions, and thanks for creating and maintaining this fork!

@wonderwhy-er
Copy link
Collaborator

I do get errors in master version and it works after this fix

But I remember testing it before.
In reality there is this method below that does similar thing
initializeModelList

And its called in some places like
https://github.com/coleam00/bolt.new-any-llm/blob/main/app/components/chat/BaseChat.tsx#L134

In theory, after your change it can be removed
I like your solution better, should error less

Though I am already thinking that we should change how this works.
We should have dynamic models list per provider be called in lazy fashion when provider is called for the first time.
And it should probably be filtered/paged/searched or soemthing.
Providers like huggingface have a LOT of models in their model list.

But that is for future.

Here we do need this fix quicker as its a bug.

Can you remove initializeModelList completely from codebase and test?

@wonderwhy-er wonderwhy-er self-requested a review November 20, 2024 14:00
@chrismahoney
Copy link
Collaborator

I agree with @wonderwhy-er, 👍 to merge after the requested change and test. Thanks!

@coleam00
Copy link
Collaborator

@ssmirr Let us know when you get to the comments above! Thank you for this!

@dustinwloring1988 dustinwloring1988 added the question Further information is requested label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use any Ollama models, although Ollama itself is working fine.
5 participants