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

feature: implement _acall method for GPT4All(LLM) #14495

Conversation

khaledadrani
Copy link

@khaledadrani khaledadrani commented Dec 9, 2023

Okey, I tested locally.

I ran "make format". No issue.

I ran "make lint". I found this issue How to fix error "cannot import name 'ModelMetaclass' from 'pydantic.main'". I followed the answer pip install lightning==2.0.1 and the command ran without problem. I believe this is not related to the PR but I had to clarify this.

I ran "make test". No issue.

This is more of a fix for the existing default implementation of the _acall method inherited by the GPT4All class.

This is my first contribution ever in the open source world, so let me know if there is anything wrong.

  1. Let me know if there is a need for adding unit tests and integration tests for LLMs or chains or callbacks. I read the existing tests and found out that they cover them in a generic manner.

  2. I have a working example that showcases how to effectively use an asynchronous callback handler with the GPT4All and RetrievalQA. I did not find the directory docs/extra, should I put it in cookbooks?

Copy link

vercel bot commented Dec 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Mar 1, 2024 9:14am

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. Ɑ: models Related to LLMs or chat model modules 🤖:improvement Medium size change to existing code to handle new use-cases labels Dec 9, 2023
@khaledadrani khaledadrani changed the title feature: implement _acall method for GPT4ALL(LLM) feature: implement _acall method for GPT4All(LLM) Dec 9, 2023
run_manager: Optional[AsyncCallbackManagerForLLMRun] = None,
**kwargs: Any,
) -> str:
r"""Asynchronous Call out to GPT4All's generate method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r"""Asynchronous Call out to GPT4All's generate method.
"""Asynchronous Call out to GPT4All's generate method.

text_callback = partial(run_manager.on_llm_new_token, verbose=self.verbose)

text = ""
for token in self.client.generate(prompt, **self._default_params()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably wants to call agenerate right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably wants to call agenerate right?

agenerate is not found for GPT4All?

AttributeError: 'GPT4All' object has no attribute 'agenerate'

What I meant to do, is using the streaming of the gpt4all model while awaiting the callback from the async callback handler

@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from 7c09169 to 30d7e85 Compare December 14, 2023 18:25
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. size:M This PR changes 30-99 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. size:XS This PR changes 0-9 lines, ignoring generated files. labels Dec 14, 2023
@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from cfe3ebd to 7c84125 Compare December 14, 2023 18:47
@khaledadrani
Copy link
Author

@hwchase17 the logic of gpt4all was changed to langchain-community, right?

@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from 7c84125 to b23e2ec Compare December 14, 2023 18:52
@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from b23e2ec to efaa8e9 Compare December 22, 2023 13:30
@khaledadrani
Copy link
Author

@hwchase17 could you please review again?

text_callback = partial(run_manager.on_llm_new_token, verbose=self.verbose)

text = ""
for token in self.client.generate(prompt, **self._default_params()):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just making sure, the client doesn't have any async generation method does it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming you are referring to agenerate. I already made a previous comment about this.

agenerate is not found for GPT4All?

AttributeError: 'GPT4All' object has no attribute 'agenerate'

I will double check now, maybe some updates occured.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked now, the same issue if I try to replace generate by agenerate "AttributeError: 'GPT4All' object has no attribute 'agenerate'"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baskaryan I am working on a cookbook too, I will add it to show how this feature works.
However, I have a question:
if I use this:

from langchain.llms.gpt4all import GPT4All

model_path = ".../mistral-7b-openorca.Q4_0.gguf"
model = GPT4All(model=model_path,
               callbacks=streaming_callbacks,
               verbose=False)
await model._acall(prompt="tell me about Python", n_predict=55)
# returns "\n" only

But if I integrated it in the RetrievalQA, it works with acall:

qa = RetrievalQA.from_chain_type(llm=model, chain_type="stuff", retriever=vector_retriever_client,
                                              return_source_documents=True)
model_response = await qa.acall(
                inputs={"query": "what is Python?"}
            )

I am still working in the notebook and I did not understand this behavior. This is also the exact situation I encountered in a professional project that drove me to make this PR.

@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from efaa8e9 to 1767973 Compare January 30, 2024 08:54
@hwchase17 hwchase17 closed this Jan 30, 2024
@baskaryan baskaryan reopened this Jan 30, 2024
@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from 688e3c5 to f7f4f0d Compare February 5, 2024 09:52
@@ -211,3 +214,40 @@ def _call(
if stop is not None:
text = enforce_stop_tokens(text, stop)
return text

async def _acall(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't LLM._acall have a default implementation that does the same thing

cc @nfcampos

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, that is true. It has that method implemented. I tested it also and it works the same as this one.
My PR is linked to this issue #5210, if the issue is solved already, then close both the issue and this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the current implementation for GPT4All tested with websockets for example (to test the asynchronous behavior works as intended?) (I will double check this ASAP)

@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from a4a2431 to f7f4f0d Compare February 12, 2024 08:16
chore: remove incorrect docstring part

refactor: remove the acall since gpt4all was moved to langchain-community

refactor: update fix location to gpt4all from langchain to langchain-community

refactor: clean docstring
@khaledadrani khaledadrani force-pushed the feature/gpt4all-async-support branch from f7f4f0d to 54c0975 Compare March 1, 2024 09:14
@khaledadrani
Copy link
Author

khaledadrani commented Mar 7, 2024

user @charlod confirmed that the current ainvoke works. So this issue can be closed #5210

@baskaryan
Copy link
Collaborator

user @charlod confirmed that the current ainvoke works. So this issue can be closed #5210

closing per ^

@baskaryan baskaryan closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases Ɑ: models Related to LLMs or chat model modules size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants