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 APIBrainQA #1606

Merged
merged 4 commits into from
Nov 8, 2023
Merged

feat: add APIBrainQA #1606

merged 4 commits into from
Nov 8, 2023

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Nov 7, 2023

Issue: #1566

@dosubot dosubot bot added the area: backend Related to backend functionality or under the /backend directory label Nov 7, 2023
Copy link

vercel bot commented Nov 7, 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 Nov 8, 2023 0:26am
quivr-strapi ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2023 0:26am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2023 0:26am

Copy link
Contributor

github-actions bot commented Nov 7, 2023

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

The code seems to be well-structured and follows the SOLID principles. However, there are a few areas that could be improved for better error handling and readability:

  1. The generate_stream method has multiple raise Exception statements. It would be better to raise more specific exceptions to provide clearer error messages.

  2. The generate_stream method is quite long and does a lot of things. Consider breaking it down into smaller methods to improve readability and maintainability.

  3. The use of **kwargs in the __init__ method could potentially lead to unexpected behavior if an unexpected keyword argument is passed. Consider explicitly listing out all the parameters that the method expects.


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

The code is generally well-written, but there are a few areas that could be improved:

  1. The get_answer_generator method could benefit from more specific error handling. Instead of raising a generic Exception when the brain is not found, consider raising a custom exception.

  2. The get_answer_generator method is quite long and does a lot of things. Consider breaking it down into smaller methods to improve readability and maintainability.


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

The code is generally well-written, but there are a few areas that could be improved:

  1. The call_brain_api function could benefit from more specific error handling. Instead of raising a generic Exception when the brain definition is not found, consider raising a custom exception.

  2. The function call_brain_api 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



def get_api_brain_definition_as_json_schema(brain: BrainEntity):
if not brain:
Copy link
Contributor

Choose a reason for hiding this comment

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

How can it be "not brain" if the type is BrainEntity ?


required = []
required.extend(api_brain_definition.params.required)
required.extend(api_brain_definition.search_params.required)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of the extend !

class ApiBrainDefinitionSchemaProperty(BaseModel):
type: str
description: str
enum: list
Copy link
Contributor

Choose a reason for hiding this comment

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

whats this enum for ?

@@ -32,6 +32,7 @@ def get_answer_generator(
user_openai_api_key,
streaming,
prompt_id,
user_id,
):
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the user_id and brain_id to be attributes of the class BrainlessChat and BrainfullChat

@@ -7,6 +7,15 @@
from routes.authorizations.types import RoleEnum
from routes.chat.interface import ChatInterface

models_supporting_function_calls = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice !

@@ -1,7 +1,8 @@
from uuid import UUID

from models import get_supabase_client
from utils import build_secret_unique_name

from repository.external_api_secret.utils import build_secret_unique_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from repository.external_api_secret.utils import build_secret_unique_name
from utils import build_secret_unique_name

Copy link
Contributor

@gozineb gozineb left a comment

Choose a reason for hiding this comment

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

Great work mamadz, just leaving a few comments to discuss later on :)

@gozineb gozineb merged commit addcd27 into main Nov 8, 2023
mamadoudicko pushed a commit that referenced this pull request Nov 13, 2023
🤖 I have created a release *beep* *boop*
---


## 0.0.109 (2023-11-13)

## What's Changed
* feat: add APIBrainQA by @mamadoudicko in
#1606
* feat: allow users to chat with apis by @mamadoudicko in
#1612
* feat(docker): use multi-stage Docker builds for smaller images by
@shidenkai0 in #1614

## New Contributors
* @shidenkai0 made their first contribution in
#1614

**Full Changelog**:
v0.0.108...v0.0.109

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
coolCatalyst added a commit to coolCatalyst/quivr that referenced this pull request Jun 1, 2024
🤖 I have created a release *beep* *boop*
---


## 0.0.109 (2023-11-13)

## What's Changed
* feat: add APIBrainQA by @mamadoudicko in
QuivrHQ/quivr#1606
* feat: allow users to chat with apis by @mamadoudicko in
QuivrHQ/quivr#1612
* feat(docker): use multi-stage Docker builds for smaller images by
@shidenkai0 in QuivrHQ/quivr#1614

## New Contributors
* @shidenkai0 made their first contribution in
QuivrHQ/quivr#1614

**Full Changelog**:
QuivrHQ/quivr@v0.0.108...v0.0.109

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants