-
Notifications
You must be signed in to change notification settings - Fork 333
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
Conversation
There was a problem hiding this 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 in3
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 iningest_files_app
,update_files_app
, andingest_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.
There was a problem hiding this 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 in1
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 thatcollection_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.
There was a problem hiding this 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 in4
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 convertingcollection_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 thesendgrid
dependency in thepyproject.toml
file seems unrelated to the main focus of this PR, which is about convertingcollection_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 functiontest_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.
There was a problem hiding this 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 in1
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.
There was a problem hiding this 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 in2
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 usingresolves.toBeDefined()
or a more specific matcher instead ofresolves.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 ofresolves.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 usingresolves.toBeDefined()
or a more specific matcher instead ofresolves.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 ofresolves.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.
Important
Convert
collection_ids
to strings iningestion_router.py
and update Docker Compose files for PostgreSQL port configuration.collection_ids
to string format iningest_files_app
,update_files_app
, andingest_chunks_app
iningestion_router.py
.compose.full.yaml
,compose.yaml
) to allow PostgreSQL port configuration usingR2R_POSTGRES_PORT
.PGPORT
environment variable incompose.full.yaml
andcompose.yaml
for PostgreSQL service.sendgrid
dependency topyproject.toml
.runner_sdk_basic.py
to correctly handle message role and content checks.This description was created by for cd28437. It will automatically update as commits are pushed.