-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix(community) : Upgrade node-llama-cpp to be compatible with version 3 #7135
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
@jacoblee93 was wondering if it was possible for you to take a look at what we have so far for our implementation, it would be greatly appreciated. |
@@ -2,7 +2,7 @@ import { LlamaCppEmbeddings } from "@langchain/community/embeddings/llama_cpp"; | |||
|
|||
const llamaPath = "/Replace/with/path/to/your/model/gguf-llama2-q4_0.bin"; | |||
|
|||
const embeddings = new LlamaCppEmbeddings({ | |||
const embeddings = await LlamaCppEmbeddings.embeddingsInit({ |
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.
Would prefer these all to be named .initialize
for consistency
_llmType() { | ||
return "llama2_cpp"; | ||
return "llama3_cpp"; |
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.
Let's just call it llama_cpp
@@ -87,20 +91,35 @@ export class ChatLlamaCpp extends SimpleChatModel<LlamaCppCallOptions> { | |||
return "ChatLlamaCpp"; | |||
} | |||
|
|||
constructor(inputs: LlamaCppInputs) { | |||
private constructor(inputs: LlamaCppInputs) { |
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.
Does this make inheritance impossible? Should it be protected
instead?
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.
And is there no sync API anymore?
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.
Unfortunately, there isn't a synchronous API anymore so the current implementation is the best workaround right now. This is both outlined in the docs and the source code. And yea it does make inheritance impossible, so will change to protected, as long as the person using the library is unable to access the constructor directly, all should be fine.
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.
Haven't run through it yet but looks great overall! Some comments on naming - can you clean up?
Addressing Comments Made to PR
Made constructors of node-llama-cpp public
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.
Seems to run for me, thank you!
changed docs
Updating Forked Version With Main
Just one merge to main should be fine! If you keep doing it, it requires CI to run again. |
[Work in Progress]
Draft PR to address the following issue: #6994
Using asynchronous function to load models from node-llama-cpp breaks adherence to the default way of instantiating components ( chatmodels, embeddings, llms, etc ).
Here is an example:
before with node-llama-cpp v2
after with node-llama-cpp v3
Question for the maintainers:
Our implementation changes the way LlamaCpp model is instantiated. Is it this fine moving forward?