insert human eval scores to clickhouse #163
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 76e7a98 in 1 minute and 6 seconds
More details
- Looked at
114lines of code in4files - Skipped
1files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:20
- Draft comment:
Consider adding error handling for the database query operation to handle potential failures. - Reason this comment was not posted:
Comment was on unchanged code.
2. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:27
- Draft comment:
Consider adding error handling for the database query operation to handle potential failures. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:44
- Draft comment:
Consider adding error handling for the database query operation to handle potential failures. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_m2fxoGvTC8SSFdkJ
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.
| } | ||
| }); | ||
| if (evaluation && evaluationValues.length > 0) { | ||
| const result = await clickhouseClient.insert({ |
There was a problem hiding this comment.
Consider adding error handling for the ClickHouse insert operation to handle potential failures.
| }); | ||
| } | ||
| } | ||
|
|
||
| await db.insert(evaluationScores).values(evaluationValues); |
There was a problem hiding this comment.
Consider adding error handling for the database insert operation to handle potential failures.
frontend/.env.local.example
Outdated
| @@ -25,3 +25,6 @@ AWS_REGION= | |||
| AWS_ACCESS_KEY_ID= | |||
| AWS_SECRET_ACCESS_KEY= | |||
| S3_IMGS_BUCKET= | |||
|
|
|||
| CLICKHOUSE_URL=http://localhost:8123 | |||
There was a problem hiding this comment.
The CLICKHOUSE_URL constant is already defined in app-server/.env.example, docker-compose-full.yml, and docker-compose-local-build.yml. Consider reusing the existing configuration to avoid duplication.
- constant
CLICKHOUSE_URL(.env.example) - constant
CLICKHOUSE_URL(docker-compose-full.yml) - constant
CLICKHOUSE_URL(docker-compose-local-build.yml)
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2d5184e in 25 seconds
More details
- Looked at
56lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/.env.local.example:25
- Draft comment:
Consider addingCLICKHOUSE_URLandCLICKHOUSE_USERback to the.env.local.examplefile to ensure consistency with the docker-compose files and avoid potential errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The removal ofCLICKHOUSE_URLandCLICKHOUSE_USERseems intentional due to theENVIRONMENT=LITEsetting, which disables ClickHouse dependencies. The comment does not provide strong evidence that these variables are still necessary. It seems speculative and not actionable, as the removal aligns with the current environment setting.
I might be missing the context of how the docker-compose files are structured and whether they still require these variables. However, the current file setup suggests they are not needed.
The presence ofENVIRONMENT=LITEstrongly indicates that ClickHouse is not required, making the comment less relevant. Without evidence of a dependency, the comment should be removed.
The comment should be deleted as it lacks strong evidence of necessity and seems speculative given the current environment setting.
Workflow ID: wflow_MwxDMaue1tQ0qKTq
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a19833d in 11 seconds
More details
- Looked at
35lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/lib/clickhouse/client.ts:8
- Draft comment:
The removal of theisFeatureEnabled(Feature.FULL_BUILD)check means the ClickHouse client will always be created with the async insert settings. Ensure this is the intended behavior, as it changes the previous conditional logic. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_1zLdI1Twi2nThVHm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Inserts human evaluation scores into ClickHouse when
Feature.FULL_BUILDis enabled, with new client and environment configurations.route.tsifFeature.FULL_BUILDis enabled.clickhouseClientto insert data intoevaluation_scorestable.CLICKHOUSE_URLandCLICKHOUSE_USERto.env.local.example.docker-compose-full.ymlanddocker-compose-local-build.ymlto include ClickHouse environment variables.@clickhouse/clienttopackage.jsondependencies.clickhouseClientinclient.tsconfigured for asynchronous inserts ifFeature.FULL_BUILDis enabled.This description was created by
for a19833d. It will automatically update as commits are pushed.