Conversation
ArticleFactChecker had no wall-clock timeout, risking unbounded execution on articles with many claims. Add asyncio.wait_for-based overall timeout (default 900s) with input validation, range clamping [30s, 7200s], and graceful error reporting on both normal and Jupyter fallback paths. Also add missing arxiv>=2.4.0 dependency to requirements/agent.txt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
feat: table、equation质检prompt
feat: BaseTextQuality根据score来判断
* metric: update LLMTextQualityV5 * 📚 Auto-update metrics documentation --------- Co-authored-by: GitHub Action <action@github.com>
feat: EvaluatorLLMArgs删除parameters属性，允许扩展
docs: update wechat (#378)
![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 the configuration system by flattening the nested parameters dictionary into Pydantic's model_extra across all evaluators, documentation, and examples. It also introduces new metrics (LLMTextEquation, LLMTextTable), enhances LLMTextQualityV5 guidelines, and implements an asynchronous overall_timeout mechanism for ArticleFactChecker. Feedback focuses on critical safety and concurrency issues: several locations risk a TypeError if model_extra is None (requiring safe access or default dictionaries), and direct modification of class attributes like dynamic_config or strictness during evaluation poses thread-safety risks and state leakage. Additionally, copy-paste errors were identified in the metadata of the new metric classes.
| if cls.dynamic_config.parameters is None: | ||
| cls.dynamic_config.parameters = {} | ||
| cls.dynamic_config.parameters.setdefault("temperature", 0) | ||
| if 'temperature' not in cls.dynamic_config.model_extra: |
There was a problem hiding this comment.
Accessing model_extra directly can raise a TypeError if no extra fields were provided during initialization, as Pydantic v2 sets model_extra to None in that case. It is safer to use (cls.dynamic_config.model_extra or {}).
| if 'temperature' not in cls.dynamic_config.model_extra: | |
| if 'temperature' not in (cls.dynamic_config.model_extra or {}): |
| cls.dynamic_config.parameters = {} | ||
| cls.dynamic_config.parameters.setdefault("temperature", 0) | ||
| if 'temperature' not in cls.dynamic_config.model_extra: | ||
| cls.dynamic_config.temperature = 0 |
There was a problem hiding this comment.
Modifying cls.dynamic_config directly is not thread-safe and leads to state leakage across different evaluation tasks because dynamic_config is a class attribute shared by all instances of the evaluator. Consider using a local copy of the parameters for the current evaluation context.
| max_tokens=params.get("max_tokens", 4000) if params else 4000, | ||
| presence_penalty=params.get("presence_penalty", 0) if params else 0, | ||
| frequency_penalty=params.get("frequency_penalty", 0) if params else 0, | ||
| **extra_params, |
There was a problem hiding this comment.
Unpacking extra_params with ** will raise a TypeError if extra_params is None. In Pydantic v2, model_extra is None if no extra fields are present. Use **(extra_params or {}) to ensure it is always a mapping.
| **extra_params, | |
| **(extra_params or {}), |
| current_params = cls.dynamic_config.parameters or {} | ||
| current_params['temperature'] = 0.7 | ||
| cls.dynamic_config.parameters = current_params | ||
| if hasattr(cls, 'dynamic_config') and 'temperature' not in cls.dynamic_config.model_extra: |
There was a problem hiding this comment.
This check will fail with a TypeError if cls.dynamic_config.model_extra is None. Use (cls.dynamic_config.model_extra or {}) to safely check for the existence of the 'temperature' key.
| if hasattr(cls, 'dynamic_config') and 'temperature' not in cls.dynamic_config.model_extra: | |
| if hasattr(cls, 'dynamic_config') and 'temperature' not in (cls.dynamic_config.model_extra or {}): |
| cls.strictness = cls.dynamic_config.parameters.get('strictness', 3) | ||
| if hasattr(cls, 'dynamic_config'): | ||
| threshold = cls.dynamic_config.model_extra.get('threshold', 5) | ||
| cls.strictness = cls.dynamic_config.model_extra.get('strictness', 3) |
There was a problem hiding this comment.
Modifying cls.strictness (a class attribute) inside the eval method causes state leakage between different evaluation tasks and is not thread-safe. Use a local variable for the strictness value used in the current evaluation.
| # Metadata for documentation generation | ||
| _metric_info = { | ||
| "category": "Pretrain Text Quality Assessment Metrics", | ||
| "metric_name": "LLMTextQualityV5", |
There was a problem hiding this comment.
The metric_name in _metric_info is set to "LLMTextQualityV5", which appears to be a copy-paste error from the LLMTextQualityV5 class. It should be updated to "LLMTextEquation" to correctly identify this metric in reports.
| "metric_name": "LLMTextQualityV5", | |
| "metric_name": "LLMTextEquation", |
| # Metadata for documentation generation | ||
| _metric_info = { | ||
| "category": "Pretrain Text Quality Assessment Metrics", | ||
| "metric_name": "LLMTextQualityV5", |
There was a problem hiding this comment.
The metric_name in _metric_info is set to "LLMTextQualityV5", which appears to be a copy-paste error. It should be updated to "LLMTextTable" for accurate reporting and metadata.
| "metric_name": "LLMTextQualityV5", | |
| "metric_name": "LLMTextTable", |
3 participants
No description provided.