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

feat(microservices): split into microservice quivr to better handle long services #972

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

StanGirard
Copy link
Collaborator

@StanGirard StanGirard commented Aug 17, 2023

Split Quivr into 4 microservices for now:

  • Core
  • Upload
  • Crawl
  • Chat

Only in local while i get AWS ready for the change

image

@StanGirard StanGirard temporarily deployed to preview August 17, 2023 17:51 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented Aug 17, 2023

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 18, 2023 8:10am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 18, 2023 8:10am

@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2023

Risk Level 2 - /home/runner/work/quivr/quivr/backend/core/chat_service.py

The code is generally well-written and follows good practices. However, there are a few areas that could be improved for better readability and maintainability:

  1. The SENTRY_DSN environment variable is fetched directly using os.getenv(). It would be better to handle the case where this environment variable is not set. This can be done by providing a default value like os.getenv('SENTRY_DSN', 'default_value') or by raising an error if it's not set.

  2. The handle_request_validation_error function is defined but not used anywhere in the code. If it's not needed, consider removing it to avoid confusion.

  3. The if __name__ == '__main__': block is used to run the application. This is fine for a script, but if this file is part of a larger application, consider moving the application running logic to a separate script.


Risk Level 2 - /home/runner/work/quivr/quivr/backend/core/routes/chat_routes.py

  1. The os.getenv function is used to get the value of the MAX_REQUESTS_NUMBER environment variable. If this variable is not set, the default value is 1. This could potentially limit the number of requests a user can make to 1 per day, which might be too restrictive. Consider increasing the default value or ensuring that the environment variable is always set.

  2. The delete_chat_from_db function catches all exceptions and simply prints them. This could make debugging difficult in the future as the exceptions are not re-raised. Consider logging the exceptions and re-raising them.

  3. The check_user_limit function increments the user's request count even if the user's OpenAI API key is None. This could potentially lead to incorrect request counts. Consider moving the increment operation to the else block.

if user.user_openai_api_key is None:
    date = time.strftime(\"%Y%m%d\")
    max_requests_number = int(os.getenv(\"MAX_REQUESTS_NUMBER\", 1))
else:
    user.increment_user_request_count(date)
    if int(user.requests_count) >= int(max_requests_number):
        raise HTTPException(
            status_code=429,
            detail=\"You have reached the maximum number of requests for today.\",
        )

Risk Level 2 - /home/runner/work/quivr/quivr/backend/core/llm/qa_base.py

The code is generally well-written and follows good practices. However, there are a few areas that could be improved for better readability and maintainability:

  1. The openai_api_key is used directly in the _create_llm function. It would be better to handle the case where this key is not set. This can be done by providing a default value or by raising an error if it's not set.

  2. The verbose parameter is set to False in the _create_llm and ConversationalRetrievalChain functions. If you want to make your code more flexible, consider making verbose a parameter of the function so that it can be changed based on the context.

  3. The generate_stream function is quite long and does a lot of things. Consider breaking it down into smaller functions to improve readability and maintainability.


🔧🔍📚


Powered by Code Review GPT

@StanGirard StanGirard changed the title Feat/stans night feat(microservices): split into microservice quivr to better handle long services Aug 17, 2023
@StanGirard StanGirard temporarily deployed to preview August 18, 2023 08:08 — with GitHub Actions Inactive
@StanGirard StanGirard merged commit 7281fd9 into main Aug 18, 2023
StanGirard added a commit that referenced this pull request Sep 12, 2023
#972)

* feat(verbose): removed

* feat(microservices): split quivr into micro services

* test(main): fixed
@riccardolinares
Copy link
Contributor

Is it ready for aws? Can you share how to setup the services on aws?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants