Skip to content

fix: Isolate evaluator dynamic configs#396

Merged
e06084 merged 1 commit intoMigoXLab:mainfrom
daniel5u:main
Apr 29, 2026
Merged

fix: Isolate evaluator dynamic configs#396
e06084 merged 1 commit intoMigoXLab:mainfrom
daniel5u:main

Conversation

@daniel5u
Copy link
Copy Markdown
Contributor

背景

此前配置注入会直接复用 evaluator 类上的 dynamic_config 对象。
当多个 evaluator 实例分别设置不同配置时,后一次配置可能覆盖前一次配置,并污染类上的默认配置。

本 PR 先修复配置对象共享污染问题,为后续支持 LLMCustomRule 做基础准备。

变更内容

  • Model.set_config_rule() 中,应用用户配置前先 deepcopy 默认 dynamic_config
  • Model.set_config_llm() 中,同样先 deepcopy 默认 dynamic_config,再应用用户配置。
  • 新增 rule / LLM evaluator 配置隔离单测。

测试

python -m pytest -q test/scripts/model/test_modelres.py test/scripts/model/test_model_config_isolation.py

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request ensures configuration isolation by performing deep copies of dynamic configurations when setting rules and LLMs, preventing unintended shared state between objects. I have suggested using Pydantic's built-in model_copy(deep=True) method instead of the standard library's copy.deepcopy, which is more idiomatic and efficient for Pydantic models. Adopting this approach will also allow for the removal of the unnecessary copy import.

Comment thread dingo/model/model.py Outdated
if not rule_config:
return
config_default = getattr(rule, 'dynamic_config')
config_default = copy.deepcopy(getattr(rule, 'dynamic_config'))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

建议使用 Pydantic 提供的 model_copy(deep=True) 方法来替代 copy.deepcopy(getattr(...))。由于 rule.dynamic_config 是一个 Pydantic 模型,使用其内置的拷贝方法更加高效且符合惯用法。此外,直接访问属性比使用 getattr 字符串更清晰。

Suggested change
config_default = copy.deepcopy(getattr(rule, 'dynamic_config'))
config_default = rule.dynamic_config.model_copy(deep=True)

Comment thread dingo/model/model.py Outdated
if not llm_config:
return
config_default = getattr(llm, 'dynamic_config')
config_default = copy.deepcopy(getattr(llm, 'dynamic_config'))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

同样建议在这里使用 model_copy(deep=True)。如果这两处都修改了,顶部的 import copy 也可以移除。

Suggested change
config_default = copy.deepcopy(getattr(llm, 'dynamic_config'))
config_default = llm.dynamic_config.model_copy(deep=True)

@e06084 e06084 merged commit 0db9613 into MigoXLab:main Apr 29, 2026
3 checks passed
e06084 added a commit that referenced this pull request Apr 30, 2026
* docs: update wechat (#390)

* docs: update wechat (#392)

* docs: update wechat (#395)

* Isolate evaluator dynamic configs (#396)

---------

Co-authored-by: sjshailab <shijinpjlab@163.com>
Co-authored-by: daniel5u <danielsuuuuuu@gmail.com>
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.

2 participants