Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures configuration isolation by deep copying dynamic_config when setting rules or LLM configurations, and adds corresponding unit tests. Feedback was provided to avoid using model_dump() when iterating over configuration arguments, as it converts nested Pydantic models into dictionaries and leads to attribute access errors; using dict().items() is recommended instead to preserve object types.
| config_default = rule.dynamic_config.model_copy(deep=True) | ||
| # Iterate over rule_config fields using Pydantic's model_dump() | ||
| for k, v in rule_config.model_dump().items(): | ||
| if v is not None: |
There was a problem hiding this comment.
Using model_dump() on a Pydantic model converts all nested models into dictionaries. When these dictionaries are later assigned back to the configuration object via setattr, the nested fields lose their Pydantic model type and become plain dicts. This will cause an AttributeError when the code attempts to access attributes on those nested objects (e.g., config.embedding_config.model). To preserve the object types, iterate over the model instance directly or use dict(model).items() which performs a shallow dump.
| config_default = rule.dynamic_config.model_copy(deep=True) | |
| # Iterate over rule_config fields using Pydantic's model_dump() | |
| for k, v in rule_config.model_dump().items(): | |
| if v is not None: | |
| config_default = rule.dynamic_config.model_copy(deep=True) | |
| # Iterate over rule_config fields directly to preserve nested model types | |
| for k, v in dict(rule_config).items(): | |
| if v is not None: |
| config_default = llm.dynamic_config.model_copy(deep=True) | ||
| # Iterate over llm_config fields using Pydantic's model_dump() | ||
| for k, v in llm_config.model_dump().items(): | ||
| if v is not None: |
There was a problem hiding this comment.
Same as the issue in set_config_rule. EvaluatorLLMArgs contains embedding_config, which is a nested Pydantic model. Using model_dump() will turn it into a dictionary, breaking subsequent attribute access on the configuration object. Iterating over dict(llm_config).items() is safer as it preserves nested objects and correctly handles extra fields allowed by the model configuration.
| config_default = llm.dynamic_config.model_copy(deep=True) | |
| # Iterate over llm_config fields using Pydantic's model_dump() | |
| for k, v in llm_config.model_dump().items(): | |
| if v is not None: | |
| config_default = llm.dynamic_config.model_copy(deep=True) | |
| # Iterate over llm_config fields directly to preserve nested model types | |
| for k, v in dict(llm_config).items(): | |
| if v is not None: |
No description provided.