fix: inject llm config into class in local executor#403
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces thread-safe state management for LLM evaluations within the LocalExecutor by implementing class-level locking and state capture/restoration mechanisms. This ensures that dynamic configurations remain isolated during parallel execution. Review feedback highlights a significant performance bottleneck caused by the serialization of LLM calls via per-class locks and suggests refactoring to instance-based models. Additionally, it was noted that repeated client initialization adds unnecessary overhead and that a missing check for unregistered LLM names could lead to runtime errors.
| lock = cls._get_llm_lock(model_cls) | ||
| with lock: |
There was a problem hiding this comment.
The introduction of a per-class lock (cls._get_llm_lock(model_cls)) effectively serializes all evaluation calls for a given LLM class. In a multi-threaded environment (like LocalExecutor with max_workers > 1), this will cause a significant performance bottleneck if most tasks use the same LLM provider (e.g., OpenAI). While this ensures state isolation for the current class-based LLM implementation, it negates the benefits of parallel execution for LLM tasks. Consider if the LLM classes can be refactored to be instance-based and thread-safe to avoid this serialization.
| model_cls.client = None | ||
| if hasattr(model_cls, "embedding_client"): | ||
| model_cls.embedding_client = None | ||
| if hasattr(model_cls, "embedding_model"): | ||
| model_cls.embedding_model = None | ||
| return model_cls.eval(Data(**map_data)) | ||
| finally: | ||
| cls._restore_llm_class_state(model_cls, previous_state) |
There was a problem hiding this comment.
The current implementation resets model_cls.client to None before evaluation and restores the previous_state in the finally block. This means that any client initialized during model_cls.eval is discarded and never reused for subsequent calls, even if the configuration is identical. This leads to unnecessary overhead from repeated client initialization. Consider only resetting the client if the configuration has actually changed, and potentially caching clients based on their configuration to improve efficiency.
| model_cls = Model.llm_name_map.get(e_c_i.name) | ||
| model = model_cls() # 实例化类为对象,避免多线程配置覆盖 | ||
| Model.set_config_llm(model, e_c_i.config) | ||
| tmp = self._evaluate_llm_with_class_state(model_cls, e_c_i.config, map_data) |
There was a problem hiding this comment.
Model.llm_name_map.get(e_c_i.name) can return None if the provided LLM name is not registered. Calling _evaluate_llm_with_class_state with None will result in an AttributeError when trying to access dynamic_config. A check should be added to handle cases where the model class is not found.
if model_cls is None:
raise ValueError(f"LLM model '{e_c_i.name}' not found in llm_name_map")
tmp = self._evaluate_llm_with_class_state(model_cls, e_c_i.config, map_data)9b48cf4 to
a7186dd
Compare
Base branch: origin/dev\n\nSummary:\n- set LLM evaluator config on both the instance and class before local execution\n- fixes built-in classmethod LLM evaluators reading the default class dynamic_config and seeing an empty key\n- keeps CustomLLMMetric structured config parsing intact\n\nVerification:\n- PYTHONPATH=/Users/danielsu/intern/shAILab/dingo PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run pytest -q test/scripts/model/llm/test_llm_custom_metric.py test/scripts/model/test_model_config_isolation.py\n\nNotes:\n- This replaces the previous runtime-class isolation approach with the smaller class-config injection fix for validation.