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

search updates #14

Merged
merged 4 commits into from
Aug 22, 2024
Merged

search updates #14

merged 4 commits into from
Aug 22, 2024

Conversation

prasmussen15
Copy link
Collaborator

@prasmussen15 prasmussen15 commented Aug 22, 2024

🚀 This description was created by Ellipsis for commit a159cda

Summary:

Refactored search functionality, introduced hybrid_search, updated Graphiti class, consolidated index creation, and updated tests.

Key points:

  • Refactored search functionality into core/search directory.
  • Introduced hybrid_search function in core/search/search.py.
  • Updated Graphiti class in core/graphiti.py to use hybrid_search.
  • Consolidated index and constraint creation into build_indices_and_constraints in core/utils/maintenance/graph_data_operations.py.
  • Removed build_indices and search methods from Graphiti class.
  • Updated imports to reflect new module structure.
  • Fixed typo in core/prompts/dedupe_nodes.py.
  • Updated tests in tests/tests_int_graphiti.py to reflect changes.

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.

❌ Changes requested. Reviewed everything up to f8643f1 in 58 seconds

More details
  • Looked at 561 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. core/utils/maintenance/graph_data_operations.py:3
  • Draft comment:
    LiteralString is imported but not used in this file. Consider removing it to clean up the imports.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The import statement for LiteralString is unnecessary in graph_data_operations.py since it's not used anywhere in the file.
2. core/search/search.py:40
  • Draft comment:
    The retrieve_episodes function is called without specifying last_n, which defaults to EPISODE_WINDOW_LEN. Ensure this is the intended behavior or consider passing config.num_episodes explicitly.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In search.py, the retrieve_episodes function is called without passing the last_n parameter, which defaults to EPISODE_WINDOW_LEN. This might not be the intended behavior if the user wants to specify a different number of episodes.
3. tests/graphiti_tests_int.py:71
  • Draft comment:
    Consider calling await graphiti.build_indices_and_constraints() to ensure indices and constraints are set up before running the search tests.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests a method that is not present in the code, which makes it speculative. The original method build_indices was removed, and the comment suggests a replacement that is not defined in the code. This makes the comment not actionable as it stands.
    The method build_indices_and_constraints might exist elsewhere in the codebase, and the comment could be suggesting a valid improvement. However, without evidence of its existence in the provided context, the comment remains speculative.
    The comment should be based on the code visible in the diff. Without evidence of the method's existence, the comment is speculative and not actionable.
    The comment should be removed as it suggests a speculative change without evidence of the method's existence in the provided code.

Workflow ID: wflow_6SOrSShknXW1hrHa


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.

core/search/search_utils.py Outdated 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 4696de1 in 48 seconds

More details
  • Looked at 204 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. core/search/search.py:34
  • Draft comment:
    The return type annotation should be dict[str, list[Node | Edge]] instead of dict[str, [Node | Edge]].
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The return type annotation in the search function in core/search/search.py is incorrect. It should be dict[str, list[Node | Edge]] instead of dict[str, [Node | Edge]]. This is a minor issue but should be corrected for clarity and correctness.
2. core/search/search_utils.py:332
  • Draft comment:
    Ensure float division is used for scoring calculation to avoid integer division issues.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is unnecessary because the code already uses float division. In Python, dividing an integer by another integer using the / operator results in a float. Therefore, there is no risk of integer division here.
    I might be overlooking any specific context where integer division could still occur, but given the code provided, it seems unlikely.
    The code clearly uses float division, and the comment does not add any value. It is safe to assume that the division will result in a float.
    The comment should be removed because it is unnecessary; the code already ensures float division.
3. core/search/search_utils.py:105
  • Draft comment:
    Ensure that removing DISTINCT from the query does not lead to unintended duplicate results.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative and asks the author to verify the behavior, which is against the rules. The removal of 'DISTINCT' could potentially lead to duplicate results, but the comment does not provide a clear, actionable change. It is more of a suggestion to verify behavior, which is not allowed.
    I might be overlooking the potential impact of removing 'DISTINCT', but the comment does not provide a specific code change or refactor, just a suggestion to verify behavior.
    Even if the removal of 'DISTINCT' could lead to issues, the comment does not provide a specific code change or refactor, just a suggestion to verify behavior, which is not allowed.
    The comment should be removed as it is speculative and does not provide a clear, actionable change.
4. core/search/search_utils.py:155
  • Draft comment:
    Ensure that removing DISTINCT from the query does not lead to unintended duplicate results.
  • Reason this comment was not posted:
    Marked as duplicate.
5. core/search/search_utils.py:189
  • Draft comment:
    Ensure that removing DISTINCT from the query does not lead to unintended duplicate results.
  • Reason this comment was not posted:
    Marked as duplicate.
6. core/search/search_utils.py:228
  • Draft comment:
    Ensure that removing DISTINCT from the query does not lead to unintended duplicate results.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_vyfxPlnmPnKKnylH


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.

👍 Looks good to me! Incremental review on 704a032 in 35 seconds

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

Workflow ID: wflow_oZYem1KEfnZWHL0z


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 a159cda in 24 seconds

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

Workflow ID: wflow_JVHoHon3cqZL22zf


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 facts

async def _search(self, query: str, timestamp: datetime, config: SearchConfig):
Copy link
Contributor

Choose a reason for hiding this comment

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

The _search method is defined but not used anywhere in the class. Consider removing it if it's not needed.

@prasmussen15 prasmussen15 merged commit 63b9790 into main Aug 22, 2024
3 checks passed
@prasmussen15 prasmussen15 deleted the search-updates branch August 22, 2024 18:26
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