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: add brain prompt overwriting from chat #1012

Merged
merged 1 commit into from
Aug 22, 2023
Merged

feat: add brain prompt overwriting from chat #1012

merged 1 commit into from
Aug 22, 2023

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Aug 22, 2023

Description

  • Update backend add prompt overwriting from chat for doc based and headless

@mamadoudicko mamadoudicko temporarily deployed to preview August 22, 2023 09:39 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented Aug 22, 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 22, 2023 10:51am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 22, 2023 10:51am

@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2023

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

The code changes seem to be safe and follow the SOLID principles. However, there is a potential issue with the Optional[UUID] type for prompt_id. If prompt_id is not provided, it might cause issues in the code where it's used without checking for None. Always check if prompt_id is not None before using it.

Example:

if self.prompt_id is not None:
    # use prompt_id

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

  1. The delete_chat_from_db function is catching all exceptions and simply printing them. This is not a good practice as it can make debugging difficult. It would be better to log the exceptions with more context or re-raise them after logging. For example:
try:
    supabase_db.delete_chat_history(chat_id)
except Exception as e:
    logger.error(f'Failed to delete chat history for chat_id {chat_id}: {e}')
    raise
  1. The check_user_requests_limit function is using os.getenv to get the MAX_REQUESTS_NUMBER environment variable. It would be better to use a configuration file or a configuration management system to manage these values.
  2. The create_question_handler and create_stream_question_handler functions have a lot of duplicated code. Consider refactoring this into a separate function to improve maintainability and readability.
  3. The create_question_handler and create_stream_question_handler functions are using the print function. It would be better to use a logger to log these messages.

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

  1. The code changes seem to be safe and follow the SOLID principles. However, there is a potential issue with the Optional[UUID] type for prompt_id. If prompt_id is not provided, it might cause issues in the code where it's used without checking for None. Always check if prompt_id is not None before using it.

Example:

if self.prompt_id is not None:
    # use prompt_id
  1. The prompt_to_use and prompt_to_use_id properties are using the UUID constructor directly on self.brain_id. This might cause a ValueError if self.brain_id is not a valid UUID. Consider using a try-except block to handle this potential error.

Example:

try:
    uuid_obj = UUID(self.brain_id)
except ValueError:
    # handle invalid UUID

🔍🐛🔄


Powered by Code Review GPT

@mamadoudicko mamadoudicko merged commit b967c2d into main Aug 22, 2023
7 of 8 checks passed
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.

None yet

2 participants