Update GPT4ALL integration

[Chae4ek]

Update GPT4ALL integration

GPT4ALL have completely changed their bindings. They use a bit odd implementation that doesn't fit well into base.py and it will probably be changed again, so it's a temporary solution.

Fixes #3839, #4628

[AndriyMulyar]

We are standardizing on these bindings (they are the best multiplatform option), no more large changes 😀

[Chae4ek]

thanks a lot @AndriyMulyar!
I'm confused a little with these parameters, it looks like implementation details.
And I see you have added MPT support, do you plan to keep that params the same for each model? If so it's better to remove this check I guess, just to make it work for future models without updating it every time

[AndriyMulyar]

Let's wait to merge anything until the python bindings dev chimes in

[rguo123]

Hey @Chae4ek! The parameters are needed to set the prompt context correctly. They typically will not require any changes on the user side and as far as I know work well across MPT, llama, and GPTJ models. However, I do think they are nice to have as they may change model behavior (for example, I have messed around with temp param at times to see how it impacts the generated text) and they are "technically" required inputs to the C function calls.

I'm not sure what that backend check was for but when loading a GPT4All model using the Python bindings, we will load the correct model. The only case where this isn't applied is if it's a custom model built not provided by us but built off a valid ggml/llama.cpp backend. In this case, providing model_type is needed for us to know which architecture to use. Hope this provides some clarification. Let me know if there's anything else I can help with!

[Chae4ek]

backend check was needed to pass different parameters because pygpt4all used different ones, I'll remove it for now.

logits_size, tokens_size and n_past are unclear to me.
I'm looking at the implementation rn and:

logits_size is unused actually and can be safely removed, logits.size() is used instead. Anyway it's used only in sample_top_k_top_p and evaluation functions and always has size = n_vocab 1, 2
llama.cpp has a parameter to change this to n_vocab*N. You'll probably want to add that too.

tokens_size is unused as well.

n_past is used for the current state of the model to remember a conversation as I understand it. Currently it resets every generate call. I think it should be removed at least from here. It is impossible to save this from python

the rest parameters are pretty clear, thank you for clarification)

[AndriyMulyar]

Please do rely on the auto suggested backend - we will ensure the right model gets loaded (it won't be just hard coded if statements soon) These bindings will be supported long term: we plan to be opinionated in their maintenance and more than just the three architectures will exist.

…

On Sat, May 13, 2023, 11:40 PM Alexey Nominas ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In langchain/llms/gpt4all.py <#4567 (comment)>: > values["client"] = GPT4AllModel( - model_path=values["model"], - **model_kwargs, + model_name=model_name, + model_path=model_path, I'd not rely on auto-suggested backend cause it's simply hardcoded <https://github.com/nomic-ai/gpt4all/blob/0f71cb6759f648ad210d2b4bdd56d5b3e94b8d37/gpt4all-bindings/python/gpt4all/gpt4all.py#L274> as well as the default model path <https://github.com/nomic-ai/gpt4all/blob/0f71cb6759f648ad210d2b4bdd56d5b3e94b8d37/gpt4all-bindings/python/gpt4all/gpt4all.py#L15>. someone may rename their model.bin idk why there is some logic around it ;) may be it's better to add model_path=model_path or './' because linux users might expect the path to be relative as the default behavior for shell/bash/etc. — Reply to this email directly, view it on GitHub <#4567 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJ4TBTRIB5WKLZDSSE7KNLXGBH33ANCNFSM6AAAAAAX7JDHDA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Chae4ek]

I hope it will be determined based on the actual file.
I don't understand how it's possible to do that based only on the file name

[AndriyMulyar]

Yep 😊 Happy to take suggestions on what that looks like in the gpt4all repo - inspecting the file contents makes the most sense. It doesn't make sense to implement the selection code in every downstream library!

…

On Sun, May 14, 2023, 12:10 AM Alexey Nominas ***@***.***> wrote: I hope it will be determined based on the actual file. I don't understand how it's possible to do that based only on the file name — Reply to this email directly, view it on GitHub <#4567 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJ4TBVCTYTZ7TMV533DMNDXGBLJ7ANCNFSM6AAAAAAX7JDHDA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Chae4ek]

I'd like to use the model path without splitting it into path-to-directory and file name, up to you though

[dev2049]

@rguo123 @AndriyMulyar how does it look now?

[rguo123]

Chiming in here on auto-suggest / installation path. We have a C library method for deciding correct model to load based on magic numbers for the model binaries. We haven't ported it over to the Python bindings yet but that may be a good solution temporarily. The magic number is for context size which differs for all these models.

This may not work in the future if different model architectures have the same context size but could be useful for now. I am wary of adapting it because it does make logic for deciding model more complicated than just if's on model names and will not be a universal solution in the long term.

[rguo123]

I think having filename be part of model_path is reasonable and no strong feelings there if we want to see that change in the gpt4all package. I do think keeping model filename/name and directory separate lines up with Huggingface's from_pretrained pattern more which would be my slight preference for keeping original impl. Lmk if there are stronger leanings for either side.

[rguo123]

PR looks good to me @dev2049!

[AndriyMulyar]

I think this is ready for merge pending the resolution of the above threads

[Chae4ek]

@HyunggyuJang, @rguo123, could you please double check the latest changes and resolve the above conversations?

[rguo123]

Doesn't look like I have permission to resolve conversations here? PR lgtm!

[Chae4ek]

Oh ok, I did it

[HyunggyuJang]

LGTM!

[srinicodeit]

When will this be available in Pypi as part of the new release?