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

Convert collection id to UUID, PG Vector image needs a set variable to change port #1635

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

NolanTrem
Copy link
Collaborator

@NolanTrem NolanTrem commented Nov 25, 2024

Important

Convert collection_ids to strings in ingestion_router.py and update Docker Compose files for PostgreSQL port configuration.

  • Behavior:
    • Convert collection_ids to string format in ingest_files_app, update_files_app, and ingest_chunks_app in ingestion_router.py.
    • Update Docker Compose files (compose.full.yaml, compose.yaml) to allow PostgreSQL port configuration using R2R_POSTGRES_PORT.
  • Misc:
    • Add PGPORT environment variable in compose.full.yaml and compose.yaml for PostgreSQL service.
    • Add sendgrid dependency to pyproject.toml.
    • Fix test logic in runner_sdk_basic.py to correctly handle message role and content checks.

This description was created by Ellipsis for cd28437. 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 6b35382 in 38 seconds

More details
  • Looked at 75 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/core/main/api/ingestion_router.py:204
  • Draft comment:
    Ensure that the conversion of collection IDs to strings is consistent across all functions. This change is applied in ingest_files_app, update_files_app, and ingest_chunks_app. If collection IDs are expected as strings, this is a necessary update.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_VosHkw4n5EZJPjic


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 3465fdd in 15 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/core/main/api/ingestion_router.py:425
  • Draft comment:
    Ensure that collection_ids are converted to strings consistently across all relevant functions. This change aligns with the PR's intent to handle UUIDs as strings.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_qLh0NqxBvo1P6qa5


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 5cd0dc8 in 27 seconds

More details
  • Looked at 147 lines of code in 4 files
  • Skipped 1 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. py/core/main/api/ingestion_router.py:205
  • Draft comment:
    The if-else statement for converting collection_ids to strings is unnecessarily verbose. It can be simplified for better readability.
[str(cid) for cid in collection_ids[it]] if collection_ids else None
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR description indicates changes to convert collection_ids to strings in ingestion_router.py. The changes in the code reflect this, but the formatting of the if-else statement is unnecessarily verbose. It can be simplified for better readability.
2. py/pyproject.toml:63
  • Draft comment:
    The addition of the sendgrid dependency in the pyproject.toml file seems unrelated to the main focus of this PR, which is about converting collection_ids to strings and PostgreSQL port configuration. Ensure this change is intentional.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR description mentions allowing PostgreSQL port configuration in Docker Compose files. However, the changes in the pyproject.toml file related to the sendgrid dependency seem unrelated to the PR's main focus. This might be an oversight or an unrelated change.
3. py/tests/integration/runner_sdk_basic.py:1733
  • Draft comment:
    The check for message roles and content can be simplified by directly comparing the message dictionaries.
if message[1] != messages[i]:
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test function test_conversation_history_sdk has a redundant check for message roles and content. The logic can be simplified by directly comparing the message dictionaries.

Workflow ID: wflow_triQR94WzIj8nRYt


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 6965146 in 17 seconds

More details
  • Looked at 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/tests/integration/runner_sdk_basic.py:2224
  • Draft comment:
    Remove the commented-out code at the end of the file to keep the codebase clean.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The commented-out code at the end of the file is redundant and should be removed to keep the codebase clean.

Workflow ID: wflow_2i3QdleI1MJJC9DR


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 cd28437 in 20 seconds

More details
  • Looked at 75 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. js/sdk/__tests__/r2rClientIntegrationSuperUser.test.ts:130
  • Draft comment:
    Consider using resolves.toBeDefined() or a more specific matcher instead of resolves.not.toThrow() to ensure the promise resolves to the expected value. This applies to other similar test cases in this file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test cases in both files have a common issue with the use of resolves.not.toThrow() for promises that should resolve to a specific value. This can lead to false positives if the promise resolves to an unexpected value.
2. js/sdk/__tests__/r2rClientIntegrationUser.test.ts:148
  • Draft comment:
    Consider using resolves.toBeDefined() or a more specific matcher instead of resolves.not.toThrow() to ensure the promise resolves to the expected value. This applies to other similar test cases in this file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test cases in both files have a common issue with the use of resolves.not.toThrow() for promises that should resolve to a specific value. This can lead to false positives if the promise resolves to an unexpected value.

Workflow ID: wflow_XglcpjWeem096UKz


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

@NolanTrem NolanTrem merged commit 749a9c8 into main Nov 26, 2024
11 checks passed
@NolanTrem NolanTrem deleted the Nolan/PassCollectionID branch November 30, 2024 21: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.

1 participant