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

Bug/4299 cant instantiate simplechatmodel subclass without defining agenerate #4300

Conversation

JacobFV
Copy link
Contributor

@JacobFV JacobFV commented May 7, 2023

Fixes #4299

@@ -197,6 +197,14 @@ def _generate(
generation = ChatGeneration(message=message)
return ChatResult(generations=[generation])

def _agenerate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR!

This will have to be an async method. Will defer to @agola11 on whether it makes sense to have it default to

await asyncio.get_event_loop().run_in_executor(self._generate(messages, stop=stop, run_manager=run_manager)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this needs to be async and we should not be calling blocking code in async methods. A good solution is having the default implementation for async def _agenerate call run_in_executor, as @vowelparrot suggested

Copy link
Contributor

Choose a reason for hiding this comment

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

#4701

how does this look

@vowelparrot vowelparrot requested a review from agola11 May 7, 2023 22:55
…chatmodel-subclass-without-defining-agenerate
…chatmodel-subclass-without-defining-agenerate
…chatmodel-subclass-without-defining-agenerate
@JacobFV
Copy link
Contributor Author

JacobFV commented May 9, 2023

Coming back to this tomorrow

@dosubot dosubot bot added the 🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature label Jul 14, 2023
@baskaryan
Copy link
Collaborator

believe this was resolved in #4701, let me know if i'm missing something!

@baskaryan baskaryan closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't instantiate abstract class <subclass of SimpleChatModel> with abstract method _agenerate
6 participants