Skip to content

Conversation

mixinju
Copy link

@mixinju mixinju commented Aug 2, 2025

What type of PR is this?

refactor: Optimizing Parameter Validation with thrift_validator

Check the PR title.

  • This PR title match the format: [<type>][<scope>]: <description>. For example: [fix][backend] flaky fix
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Add documentation if the current PR requires user awareness at the usage level.
  • This PR is written in English. PRs not in English will not be reviewed.

(Optional) Translate the PR title into Chinese.

[refactor]使用thrift_validator优化参数校验-减少硬编码

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:
zh(optional):
主要变更evaluator和llm相关的参数校验

(Optional) Which issue(s) this PR fixes:

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@lsy357
Copy link
Collaborator

lsy357 commented Aug 7, 2025

Thank you for your code! We will review and merge the code after our tests pass.

@mixinju
Copy link
Author

mixinju commented Aug 19, 2025

Hello, I have already resolved the compilation failure issue. Please take another look.

@mixinju
Copy link
Author

mixinju commented Aug 25, 2025

@lsy357 Could you please review this PR for any additional feedback?

@lsy357
Copy link
Collaborator

lsy357 commented Aug 25, 2025

@lsy357 Could you please review this PR for any additional feedback?

We will test the code today, and once the results are approved, we will merge it.

3: required evaluator.EvaluatorType evaluator_type (api.body='evaluator_type', go.tag='json:"evaluator_type"')
4: optional string name (api.body='name', go.tag='json:"name"') // 展示用名称
5: optional string description (api.body='description', go.tag='json:"description"') // 描述
4: optional string name (api.body='name', go.tag='json:"name"',vt.not_nil="true") // 展示用名称
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields "name" and "description" are allowed to be empty, so the "not_nil" constraint should be removed.

3: optional string description (vt.max_size = "2048"),
4: optional eval_set.EvaluationSetSchema evaluation_set_schema,
2: optional string name (vt.not_nil="true",vt.min_size = "1", vt.max_size = "255"),
3: optional string description (vt.max_size = "2048",vt.not_nil="true"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended that the "description" field does not require the "not_nil" validation.

1: required i64 workspace_id (api.body='workspace_id', api.js_conv='true', go.tag='json:"workspace_id"',vt.not_in="0")
2: required i64 evaluator_id (api.path='evaluator_id', api.js_conv='true', go.tag='json:"evaluator_id"',vt.not_in="0")
3: required string version (api.body='version',vt.min_size="1")
4: optional string description (api.body='description',vt.not_nil="true")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended that the "description" field does not require the "not_nil" validation.

2: optional i64 workspace_id (api.js_conv = 'true', go.tag = 'json:"workspace_id"',vt.not_nil="true")
3: optional EvaluatorType evaluator_type (vt.not_nil="true")
4: optional string name (vt.not_nil="true")
5: optional string description (vt.not_nil="true")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same, description optional.

6: optional bool draft_submitted
7: optional common.BaseInfo base_info
11: optional EvaluatorVersion current_version
7: optional common.BaseInfo base_info (vt.not_nil="true")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think base_info/current_version should be remained optional.

2: optional list<runtime.Message> messages (vt.min_size="1")
// 工具
3: optional list<runtime.Tool> tools
3: optional list<runtime.Tool> tools (vt.min_size="1")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tools might be len 0, should be no min_size restriction.

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.

4 participants