wip(agents-api): Add Doc sql queries #979
Open
+1,615
−398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
CI Failure Feedback 🧐(Checks updated until commit 74add36)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 6c77490 in 1 minute and 30 seconds
More details
- Looked at
848lines of code in10files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/search_docs_hybrid.py:13
- Draft comment:
The import statement forrun_concurrentlyis unnecessary since it's not used in the code. Consider removing it to clean up the imports. - Reason this comment was not posted:
Confidence changes required:10%
The import statement forrun_concurrentlyis unnecessary since it's not used in the code.
2. agents-api/agents_api/queries/docs/search_docs_hybrid.py:134
- Draft comment:
Appending an empty list totasksis incorrect. You should append a coroutine or task. Consider usingNoneor a similar placeholder if you intend to skip adding a task. - Reason this comment was not posted:
Confidence changes required:50%
The code usestasks.append([])which is incorrect for appending tasks. It should append a coroutine instead.
3. agents-api/agents_api/queries/docs/search_docs_hybrid.py:147
- Draft comment:
Appending an empty list totasksis incorrect. You should append a coroutine or task. Consider usingNoneor a similar placeholder if you intend to skip adding a task. - Reason this comment was not posted:
Confidence changes required:50%
The code usestasks.append([])which is incorrect for appending tasks. It should append a coroutine instead.
4. agents-api/agents_api/queries/docs/search_docs_hybrid.py:154
- Draft comment:
Ensure that at least one valid task is added totasksbefore callinggather. If bothtext_queryandembeddingare empty,taskswill contain only empty lists, leading to an error. - Reason this comment was not posted:
Confidence changes required:50%
The code does not handle the case where bothtext_queryandembeddingare empty, which could lead to an error when callinggather.
5. agents-api/agents_api/queries/docs/search_docs_hybrid.py:110
- Draft comment:
Theembeddingparameter should have a default value of an empty list[]instead ofNoneto avoid type issues and simplify checks. - Reason this comment was not posted:
Confidence changes required:50%
Theembeddingparameter should have a default value of an empty list instead ofNoneto avoid type issues.
6. agents-api/agents_api/queries/docs/create_doc.py:69
- Draft comment:
Consider adding more exception handling for other potential database errors, such asasyncpg.DataErrororasyncpg.SyntaxOrAccessError, to make the function more robust. - Reason this comment was not posted:
Confidence changes required:50%
Therewrap_exceptionsdecorator is used to handle exceptions, but it doesn't cover all possible exceptions that might occur during database operations.
7. agents-api/agents_api/queries/docs/delete_doc.py:43
- Draft comment:
Consider adding more exception handling for other potential database errors, such asasyncpg.DataErrororasyncpg.SyntaxOrAccessError, to make the function more robust. - Reason this comment was not posted:
Confidence changes required:50%
Therewrap_exceptionsdecorator is used to handle exceptions, but it doesn't cover all possible exceptions that might occur during database operations.
8. agents-api/agents_api/queries/docs/get_doc.py:29
- Draft comment:
Consider adding exception handling for potential database errors, such asasyncpg.DataErrororasyncpg.SyntaxOrAccessError, to make the function more robust. - Reason this comment was not posted:
Confidence changes required:50%
Therewrap_exceptionsdecorator is used to handle exceptions, but it doesn't cover all possible exceptions that might occur during database operations.
Workflow ID: wflow_0SgPnESfFL0Scwfr
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.
| return similarity | ||
|
|
||
|
|
||
| def maximal_marginal_relevance( |
There was a problem hiding this comment.
This is an exact duplicate of the existing implementation.
- function
maximal_marginal_relevance(mmr.py)
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 93673b7 in 1 minute and 3 seconds
More details
- Looked at
1195lines of code in21files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/get_doc.py:5
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. This is also applicable in other files like 'list_docs.py', 'search_docs_by_embedding.py', and 'search_docs_by_text.py'. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
2. agents-api/agents_api/queries/docs/list_docs.py:5
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. This is also applicable in other files like 'search_docs_by_embedding.py', and 'search_docs_by_text.py'. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
3. agents-api/agents_api/queries/docs/search_docs_by_embedding.py:6
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. This is also applicable in 'search_docs_by_text.py'. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
4. agents-api/agents_api/queries/docs/search_docs_by_text.py:6
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
5. agents-api/agents_api/queries/docs/create_doc.py:98
- Draft comment:
The 'org' option is removed from the owner_type Literal, but this change is not reflected in the PR description. This should be documented for clarity. This change is also applicable in 'delete_doc.py', 'get_doc.py', and 'list_docs.py'. - Reason this comment was not posted:
Confidence changes required:50%
The 'org' option is removed from the owner_type Literal in multiple files, but this change is not reflected in the PR description. This should be documented for clarity.
Workflow ID: wflow_pnbwiXEmTCJVnbrw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on dc0ec36 in 48 seconds
More details
- Looked at
939lines of code in28files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. agents-api/agents_api/queries/users/delete_user.py:59
- Draft comment:
Theasyncpg.exceptions.UniqueViolationErrorshould not be handled here as it is not relevant to delete operations. Consider removing this exception handling. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_WUrKdAU41fxw9Cd0
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.
| status_code=404, | ||
| detail="The specified developer does not exist.", | ||
| ), | ||
| asyncpg.exceptions.UniqueViolationError: partialclass( |
There was a problem hiding this comment.
The asyncpg.exceptions.UniqueViolationError should not be handled here as it is not relevant to delete operations. Consider removing this exception handling.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 831e950 in 51 seconds
More details
- Looked at
440lines of code in11files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/get_doc.py:36
- Draft comment:
Usingast.literal_evalond["content"]can be unsafe if the content is not guaranteed to be a valid Python literal. Consider using a safer method to parse or handle the content. - Reason this comment was not posted:
Comment was on unchanged code.
2. agents-api/agents_api/queries/docs/embed_snippets.py:10
- Draft comment:
vectorizer_queryis set toNone. This is a placeholder and should be replaced with an actual query before deployment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is essentially repeating information that's already explicitly stated in a TODO comment one line above. The TODO comment is more visible and serves the same purpose. This makes the PR comment redundant and not adding any new information or value.
Perhaps the comment is trying to emphasize the importance of not deploying with a None query, which could be a critical issue.
While deployment concerns are valid, the existing TODO comment already makes it clear this needs to be replaced, and deployment issues would be caught by basic testing since the function would fail immediately.
Delete the comment as it's redundant with the existing TODO comment and doesn't provide additional actionable value.
3. agents-api/agents_api/queries/entries/list_entries.py:88
- Draft comment:
Ensuresort_byanddirectionare validated and sanitized to prevent SQL injection, as they are used in string interpolation for SQL queries. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_TPwdOI1YwvBHnQ9P
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Enhancement
Description
Added comprehensive document management system with the following features:
Core document operations:
Advanced search capabilities:
List and filtering:
Performance optimizations:
Changes walkthrough 📝
9 files
__init__.py
Initialize document management module with core operationsagents-api/agents_api/queries/docs/init.py
create_doc.py
Document creation with ownership managementagents-api/agents_api/queries/docs/create_doc.py
constraints
delete_doc.py
Document deletion with ownership validationagents-api/agents_api/queries/docs/delete_doc.py
get_doc.py
Single document retrieval functionalityagents-api/agents_api/queries/docs/get_doc.py
list_docs.py
Paginated document listing with filtersagents-api/agents_api/queries/docs/list_docs.py
mmr.py
Maximal Marginal Relevance implementation for searchagents-api/agents_api/queries/docs/mmr.py
search_docs_by_embedding.py
Vector-based document search implementationagents-api/agents_api/queries/docs/search_docs_by_embedding.py
search_docs_by_text.py
Full-text document search implementationagents-api/agents_api/queries/docs/search_docs_by_text.py
search_docs_hybrid.py
Hybrid document search with score fusionagents-api/agents_api/queries/docs/search_docs_hybrid.py
Important
Add comprehensive document management system with CRUD, advanced search, and performance optimizations.
create_doc.py,delete_doc.py,get_doc.py, andlist_docs.py.search_docs_by_text.py.search_docs_by_embedding.py.search_docs_hybrid.py.mmr.py.Docmodel inDocs.pywith new fields likemodality,language,index,embedding_model, andembedding_dimensions.test_docs_queries.pyfor CRUD operations and listing.This description was created by
for 831e950. It will automatically update as commits are pushed.