-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Refactor/knowledge brain qa #2539
Refactor/knowledge brain qa #2539
Conversation
…nd maintanability
@jacopo-chevallard is attempting to deploy a commit to the Quivr-app Team on Vercel. A member of the Team first needs to authorize it. |
|
||
# Serialize the sources list | ||
|
||
async def generate_stream(self, chat_id: UUID, question: ChatQuestion, save_answer: bool = True) -> AsyncIterable: |
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.
You might have an issue in the way you Yield the result. I don't think it is an async iterable the output.
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.
Ok thanks I'll take a look
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.
@StanGirard I just pushed a new commit
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.
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.
This usually means there is an issue with an async operator somewhere. Very hard to debug.
It could be from generate_stream when called or simply in the code an async wrongly used.
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 the issue with the rate limit is probably because you didn't put your credit card on OpenAI for the api 😅
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 the issue with the rate limit is probably because you didn't put your credit card on OpenAI for the api 😅
I did, and even bought some credits to get higher RPM, but still got the same error…
I wanted to try with Ollama, but couldn’t figure out how to make it work on my local quivr…
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.
I foudn the issue :)
You are not yielding the results. I just had the same bug on another feature ;)
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.
Ah! Cool tnx!
|
Thanks for your contributions, we'll be closing this PR as it has gone stale. Feel free to reopen if you'd like to continue the discussion. |
Description
Refactoring of
backend/modules/brain/knowledge_brain_qa.py
to improve readability and maintainability. Also added docstrings.Checklist before requesting a review
Please delete options that are not relevant.
Note that I kept getting OpenAI rate limit errors (openai.RateLimitError) when testing quivr on my local machine, so I couldn't properly test the proposed changes. Please, do test the changes on a working quivr installation before merging.