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

Further cleanup, remove old deduplication logic #1746

Merged
merged 7 commits into from
Jan 1, 2025

Conversation

NolanTrem
Copy link
Collaborator

@NolanTrem NolanTrem commented Jan 1, 2025

Important

Remove old deduplication logic, update typing imports, and refactor SDK methods for improved clarity and performance.

  • Removal of Deduplication Logic:
    • Removed deduplication logic from graphs.py, kg_workflow.py, and simple/kg_workflow.py.
    • Deleted graphrag_entity_deduplication.yaml and related deduplication files.
  • Typing Imports:
    • Updated typing imports to use native types in rag.py, auth.py, and base.py.
  • SDK Updates:
    • Refactored create() and list() methods in collections.py, conversations.py, and documents.py.
    • Updated create() and delete() methods in indices.py and prompts.py.
    • Enhanced search() and completion() methods in retrieval.py.
    • Improved user management methods in users.py.
  • Misc:
    • Updated pre-commit hook in .pre-commit-config.yaml to check for Union usage.
    • Removed R2R_POSTGRES_PROJECT_NAME from compose.yaml files.

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

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 f14a65d in 1 minute and 13 seconds

More details
  • Looked at 3745 lines of code in 68 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. py/core/agent/rag.py:1
  • Draft comment:
    Consider using the | operator for union types instead of importing Union from typing for consistency with the rest of the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The PR removes the usage of the Union type from the typing module and replaces it with the | operator, which is a more modern and concise way to express union types in Python 3.10 and later. This change is consistent across multiple files, including py/core/agent/rag.py, py/core/base/providers/base.py, and others. This is a good practice as it makes the code more readable and aligns with the latest Python standards.
2. py/core/base/providers/base.py:2
  • Draft comment:
    Consider using the | operator for union types instead of importing Union from typing for consistency with the rest of the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The PR removes the usage of the Union type from the typing module and replaces it with the | operator, which is a more modern and concise way to express union types in Python 3.10 and later. This change is consistent across multiple files, including py/core/agent/rag.py, py/core/base/providers/base.py, and others. This is a good practice as it makes the code more readable and aligns with the latest Python standards.
3. py/core/base/providers/crypto.py:10
  • Draft comment:
    Consider using the | operator for union types instead of importing Union from typing for consistency with the rest of the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The PR removes the usage of the Union type from the typing module and replaces it with the | operator, which is a more modern and concise way to express union types in Python 3.10 and later. This change is consistent across multiple files, including py/core/agent/rag.py, py/core/base/providers/base.py, and others. This is a good practice as it makes the code more readable and aligns with the latest Python standards.
4. py/core/base/providers/email.py:29
  • Draft comment:
    Consider using the | operator for union types instead of importing Union from typing for consistency with the rest of the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The PR removes the usage of the Union type from the typing module and replaces it with the | operator, which is a more modern and concise way to express union types in Python 3.10 and later. This change is consistent across multiple files, including py/core/agent/rag.py, py/core/base/providers/base.py, and others. This is a good practice as it makes the code more readable and aligns with the latest Python standards.
5. py/core/base/providers/ingestion.py:5
  • Draft comment:
    Consider using the | operator for union types instead of importing Union from typing for consistency with the rest of the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The PR removes the usage of the Union type from the typing module and replaces it with the | operator, which is a more modern and concise way to express union types in Python 3.10 and later. This change is consistent across multiple files, including py/core/agent/rag.py, py/core/base/providers/base.py, and others. This is a good practice as it makes the code more readable and aligns with the latest Python standards.

Workflow ID: wflow_Z2xFghW2h6lqJxoW


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 8c0e1ae in 34 seconds

More details
  • Looked at 72 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/core/database/postgres.py:52
  • Draft comment:
    Use CryptoProviderType consistently for type hinting.
    crypto_provider: CryptoProviderType
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_gD1fGvkY0gLegyt6


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 83912b9 in 21 seconds

More details
  • Looked at 192 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/tests/unit/test_graphs.py:3
  • Draft comment:
    The import statement for 'UUID' from 'uuid' is unnecessary and can be removed since 'UUID' is not used in the code.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The import statement for 'UUID' from 'uuid' is unnecessary since 'UUID' is not used in the code.

Workflow ID: wflow_A3aIxErtWqhL4Ruh


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

@NolanTrem NolanTrem merged commit 3a2405c into main Jan 1, 2025
13 of 14 checks passed
@NolanTrem NolanTrem deleted the Nolan/RemoveDeduplication branch January 1, 2025 03:56
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