feat: RuleQuanliangFieldValidation.eval去掉classmethod#433
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors rule evaluation to use instance-level configurations instead of class-level configurations, specifically migrating RuleQuanliangFieldValidation from a class method to an instance method and updating executors and tests accordingly. To support legacy rules, a backward compatibility fallback was introduced that sets class-level configurations when classmethod-based rules are detected. Feedback highlights that this fallback is not thread-safe in multi-threaded local or Spark environments and recommends migrating all remaining rules to instance methods. Additionally, a redundant try...finally block in the test suite can now be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| eval_self = getattr(model.eval, "__self__", None) | ||
| if eval_self is model_cls: | ||
| Model.set_config_rule(model_cls, e_c_i.config) |
There was a problem hiding this comment.
Modifying the class-level configuration (model_cls) for backward compatibility is not thread-safe when using ThreadPoolExecutor (which is active when LOCAL_DEPLOYMENT_MODE is "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 eval methods (removing @classmethod) as soon as possible to ensure full thread safety.
| eval_self = getattr(model.eval, "__self__", None) | ||
| if eval_self is model_cls: | ||
| Model.set_config_rule(model_cls, e_c_i.config) |
There was a problem hiding this comment.
Similar to the local executor, modifying the class-level configuration (model_cls) for backward compatibility is not thread-safe if Spark executors are configured to run with multiple threads (e.g., when spark.executor.cores > 1 with a multi-threaded task execution model).
Classmethod-based rules should be migrated to instance methods to ensure complete thread safety across all execution environments.
| 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"])) |
There was a problem hiding this comment.
Since we are now modifying the instance-level dynamic_config of model instead of the class-level RuleQuanliangFieldValidation.dynamic_config, the class-level configuration is never mutated.
This makes the try...finally block (which saves and restores original_key_list at lines 15 and 37-38) redundant. You can simplify this test file by removing the try...finally block entirely.
1 participant
No description provided.