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: async close and multi-group search support #151

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

paul-paliychuk
Copy link
Collaborator

@paul-paliychuk paul-paliychuk commented Sep 24, 2024

Important

The PR makes close() in Graphiti asynchronous, updates search for multiple group IDs, and bumps version to 0.3.5.

  • close() in Graphiti is now asynchronous, improving resource management.
  • SearchQuery in retrieve.py accepts group_ids as a list for multi-group searches.
  • search() in retrieve.py updated for enhanced search flexibility.
  • Minor updates in zep_graphiti.py for async close().
  • Version bump in pyproject.toml from 0.3.4 to 0.3.5.

This description was created by Ellipsis for 70f64d0. It will automatically update as commits are pushed.

@ellipsis-dev ellipsis-dev bot changed the title ... feat: async close and multi-group search support Sep 24, 2024
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 8678c59 in 25 seconds

More details
  • Looked at 64 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. graphiti_core/graphiti.py:132
  • Draft comment:
    Changed close method to async and used await for closing the driver, ensuring proper asynchronous resource management.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change from synchronous to asynchronous in the close method of the Graphiti class is appropriate given the use of await in the method. This ensures that the Neo4j driver is closed asynchronously, which is consistent with the rest of the asynchronous methods in the class.
2. server/graph_service/dto/retrieve.py:9
  • Draft comment:
    Changed group_id to group_ids to support multiple group searches, aligning with the updated search functionality.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in SearchQuery from group_id to group_ids is consistent with the change in the search function call, allowing for multiple group IDs to be processed. This is a logical update to support searching across multiple groups.
3. server/graph_service/routers/retrieve.py:20
  • Draft comment:
    Updated to use query.group_ids for searching across multiple groups, consistent with the SearchQuery model change.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the search function to use query.group_ids instead of a list with a single query.group_id is consistent with the updated SearchQuery model, which now supports multiple group IDs.
4. server/graph_service/zep_graphiti.py:82
  • Draft comment:
    Changed to await client.close() to match the async close method in Graphiti, ensuring proper async resource cleanup.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in get_graphiti function to use await client.close() is consistent with the change in the close method of the Graphiti class to be asynchronous. This ensures proper resource cleanup in an asynchronous context.
5. server/graph_service/zep_graphiti.py:80
  • Draft comment:
    Avoid hardcoding sensitive information like database credentials. Consider using environment variables or a secure vault.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_nMhrMPaBI3CUVNlL


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 de526d5 in 18 seconds

More details
  • Looked at 78 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. server/graph_service/routers/retrieve.py:21
  • Draft comment:
    The use of cast here is unnecessary. query.group_ids should already be of the correct type expected by graphiti.search. Consider removing the cast.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The use of cast in line 21 is unnecessary because query.group_ids is already expected to be a list of strings, which matches the expected type in graphiti.search.
2. server/graph_service/routers/retrieve.py:1
  • Draft comment:
    Add a copyright notice at the top of the file.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The code lacks copyright headers, which is a violation of the rules.
3. graphiti_core/utils/maintenance/community_operations.py:156
  • Draft comment:
    The build_community function is doing too much. Consider refactoring it to adhere to the Single Responsibility Principle by separating concerns like summarizing pairs, generating names, and creating nodes and edges.
  • Reason this comment was not posted:
    Confidence changes required: 70%
    The function build_community is performing multiple tasks: summarizing pairs, generating a name, and creating community nodes and edges. This violates the Single Responsibility Principle.
4. server/graph_service/routers/retrieve.py:21
  • Draft comment:
    Avoid using cast to change the type of query.group_ids. Ensure the type is correctly defined in SearchQuery to match the expected type in graphiti.search.
  • Reason this comment was not posted:
    Confidence changes required: 60%
    The search function in retrieve.py is casting query.group_ids to match the expected type in graphiti.search, which is not idiomatic. The type should be handled properly without casting if possible.

Workflow ID: wflow_VbXhuhqhWKmfr2Tc


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 70f64d0 in 21 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pyproject.toml:3
  • Draft comment:
    Version update is consistent with the feature enhancement described in the PR.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The version update in pyproject.toml is consistent with the changes described in the PR, which is a minor feature enhancement.
2. pyproject.toml:1
  • Draft comment:
    No issues found in this file related to the provided rules.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The pyproject.toml file does not have any issues related to the rules provided.

Workflow ID: wflow_MRKZkmOHkxpPhzYn


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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant