-
Notifications
You must be signed in to change notification settings - Fork 74
feat: RuleQuanliangFieldValidation.eval去掉classmethod #433
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -236,8 +236,13 @@ def evaluate_item(self, eval_fields: dict, eval_type: str, map_data: dict, eval_ | |
|
|
||
| for e_c_i in eval_list: | ||
| if eval_type == 'rule': | ||
| model = Model.rule_name_map.get(e_c_i.name) | ||
| model_cls = Model.rule_name_map.get(e_c_i.name) | ||
| model = model_cls() | ||
| Model.set_config_rule(model, e_c_i.config) | ||
| # Backward compatibility for classmethod-based rules. | ||
| eval_self = getattr(model.eval, "__self__", None) | ||
| if eval_self is model_cls: | ||
| Model.set_config_rule(model_cls, e_c_i.config) | ||
|
Comment on lines
+243
to
+245
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the local executor, modifying the class-level configuration ( Classmethod-based rules should be migrated to instance methods to ensure complete thread safety across all execution environments. |
||
| elif eval_type == 'llm': | ||
| model = Model.llm_name_map.get(e_c_i.name) | ||
| Model.set_config_llm(model, e_c_i.config) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,8 +20,10 @@ def test_rule_quanliang_cases_from_jsonl(self): | |
| if not line: | ||
| continue | ||
| row = json.loads(line) | ||
| RuleQuanliangFieldValidation.dynamic_config.key_list = row["key_list"] | ||
| result = RuleQuanliangFieldValidation.eval(Data(**row["input"])) | ||
| model = RuleQuanliangFieldValidation() | ||
| model.dynamic_config = model.dynamic_config.model_copy(deep=True) | ||
| model.dynamic_config.key_list = row["key_list"] | ||
| result = model.eval(Data(**row["input"])) | ||
|
Comment on lines
+23
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are now modifying the instance-level This makes the |
||
|
|
||
| assert result.metric == "RuleQuanliangFieldValidation" | ||
| assert result.status is row["expected_status"], row["case"] | ||
|
|
||
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.
Modifying the class-level configuration (
model_cls) for backward compatibility is not thread-safe when usingThreadPoolExecutor(which is active whenLOCAL_DEPLOYMENT_MODEis"true").Multiple threads evaluating different configurations for the same classmethod-based rule will concurrently overwrite the class-level
dynamic_config, leading to race conditions and incorrect evaluation results. Since the goal of instantiating the model (model = model_cls()) was specifically to avoid multi-thread config overwrite, this backward compatibility fallback defeats that safety mechanism for any rules that still use@classmethod.Recommendation:
Migrate all remaining rules to instance-level
evalmethods (removing@classmethod) as soon as possible to ensure full thread safety.