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

fix(agents-api): Fix doc recall using search by text #506

Merged
merged 4 commits into from
Sep 19, 2024
Merged

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Sep 18, 2024

Signed-off-by: Diwank Singh Tomer [email protected]


🚀 This description was created by Ellipsis for commit b856de7

fix(agents-api): improve document recall and chat environment setup

Summary:

Enhances document recall and chat setup, updates search queries, document structures, type hints, replaces UUID4 with UUID, and updates Docker image version.

Key points:

  • Improves document recall and chat setup in agents API.
  • Updates search queries in search_docs_by_embedding.py and search_docs_by_text.py.
  • Updates document structure in chat() in chat.py to include title and content.
  • Removes unnecessary assertion in dbsf_fuse() in search_docs_hybrid.py.
  • Updates type hint for custom_api_key in acompletion() in litellm.py.
  • Replaces UUID4 with UUID in multiple router files.
  • Adds a comment in chat.py to save all choices in the response.
  • Updates Docker image version in docker-compose.yml.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 48d27a9 in 24 seconds

More details
  • Looked at 64 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. agents-api/agents_api/models/docs/search_docs_hybrid.py:45
  • Draft comment:
    Removing the assertion that checks if all text_results have a non-None distance could lead to issues if any text_result has a None distance. Ensure that all distances are valid before proceeding.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The assertion was removed, which means the code no longer checks for None distances in text_results. If a None distance is problematic, the removal could indeed lead to issues. However, the comment suggests ensuring all distances are valid, which is speculative without evidence of an actual issue. The code now defaults to 0.0 if distance is None, which might be an intended behavior change.
    I might be missing the context of why the assertion was removed. The code might handle None distances differently now, and the comment doesn't provide evidence of an actual issue.
    The code defaults to 0.0 for None distances, which might be the intended behavior. Without evidence of an issue, the comment seems speculative.
    The comment is speculative and lacks evidence of an actual issue. The code defaults to 0.0 for None distances, which might be intended. The comment should be deleted.
2. agents-api/agents_api/models/docs/search_docs_by_text.py:67
  • Draft comment:
    Using json.dumps(query) might not be necessary and could lead to incorrect query formatting. Consider using the query string directly unless JSON encoding is required by the database.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment raises a concern about the necessity and potential issues of using json.dumps(query). The code change specifically introduces this usage, so the comment is relevant to the change. However, without additional context on the database requirements, it's unclear if json.dumps(query) is indeed problematic.
    I might be missing specific knowledge about the database's query requirements. The comment could be speculative if the database indeed requires JSON formatting for queries.
    The comment directly addresses a change made in the code, which is a valid reason to consider it. However, without strong evidence that json.dumps(query) is incorrect, the comment might be speculative.
    The comment is relevant to the code change, but without strong evidence that json.dumps(query) is incorrect, it might be speculative. Therefore, the comment should be deleted.
3. agents-api/agents_api/routers/sessions/chat.py:61
  • Draft comment:
    Ensure that all necessary fields from doc_references are included in the new format when adding to env['docs']. This change modifies the structure, so verify that all required data is preserved.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR changes the way document references are added to the environment in the chat function. This change seems correct, but it's important to ensure that all necessary fields are included in the new format.

Workflow ID: wflow_RD2egUIrP9TDhnwc


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 7eddbad in 15 seconds

More details
  • Looked at 66 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_1RT4dV8J9Eg1wvFe


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

agents-api/agents_api/clients/litellm.py Show resolved Hide resolved
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on b856de7 in 27 seconds

More details
  • Looked at 1154 lines of code in 46 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. agents-api/agents_api/routers/agents/create_agent_tool.py:5
  • Draft comment:
    Duplicate import of UUID. Remove the redundant import statement.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for UUID is duplicated in several files. This is unnecessary and should be cleaned up.
2. agents-api/agents_api/routers/agents/create_or_update_agent.py:5
  • Draft comment:
    Duplicate import of UUID. Remove the redundant import statement.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for UUID is duplicated in several files. This is unnecessary and should be cleaned up.
3. agents-api/agents_api/routers/agents/list_agent_tools.py:5
  • Draft comment:
    Duplicate import of UUID. Remove the redundant import statement.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for UUID is duplicated in several files. This is unnecessary and should be cleaned up.
4. agents-api/agents_api/routers/agents/patch_agent_tool.py:5
  • Draft comment:
    Duplicate import of UUID. Remove the redundant import statement.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for UUID is duplicated in several files. This is unnecessary and should be cleaned up.

Workflow ID: wflow_KzLfAOZAulWQUTQF


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@creatorrr creatorrr merged commit 39f71a3 into dev Sep 19, 2024
4 of 7 checks passed
@creatorrr creatorrr deleted the x/fix-doc-recall branch September 19, 2024 13:47
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.

1 participant