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

Refactor/knowledge brain qa #2539

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jacopo-chevallard
Copy link

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.

  • I have performed a self-review of my code
  • I have commented hard-to-understand areas
  • I have ideally added tests that prove my fix is effective or that my feature works

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.

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label May 4, 2024
Copy link

vercel bot commented May 4, 2024

@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.

@dosubot dosubot bot added the area: backend Related to backend functionality or under the /backend directory label May 4, 2024

# Serialize the sources list

async def generate_stream(self, chat_id: UUID, question: ChatQuestion, save_answer: bool = True) -> AsyncIterable:
Copy link
Collaborator

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.

Copy link
Author

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

Copy link
Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

I still get this error :/

Don't worry I'll take a look my self, I don't want you to spend to much time on it for now ;)

Copy link
Collaborator

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.

Copy link
Collaborator

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 😅

Copy link
Author

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…

Copy link
Collaborator

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 ;)

Copy link
Author

Choose a reason for hiding this comment

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

Ah! Cool tnx!

Copy link

sonarcloud bot commented May 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend Related to backend functionality or under the /backend directory size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants