From 12090e5ab7602c7f5b0825656b1042426d5a5ffe Mon Sep 17 00:00:00 2001 From: adrian-badea Date: Fri, 17 Jul 2026 16:50:49 +0300 Subject: [PATCH] fix(eval): cache custom-coded evaluator modules instead of leaking sys.modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _create_custom_coded_evaluator_internal named the dynamically loaded module `_custom_evaluator__`. Because `data` is a fresh dict on every call, id(data) was unique each time, so the sys.modules cache never hit: the evaluator module was re-exec()'d on every call and a new module object (plus the classes/pydantic schemas it defines) was inserted into sys.modules and never removed. Any consumer building custom-coded evaluators repeatedly leaks memory and CPU without bound — building evaluators per datapoint turns an eval run into O(n^2) with multi-GB RAM growth. Key the module name on a hash of the resolved file path so sys.modules caches it: the module loads once and is reused. Pop the entry if exec fails so a broken load is not left cached (and can be retried). Add regression tests: repeated creation reuses one module (no leak), a failed load is not cached, and two files sharing a basename load as distinct modules. Co-Authored-By: Claude Opus 4.8 --- .../eval/evaluators/evaluator_factory.py | 42 +++++-- .../evaluators/test_evaluator_factory.py | 116 ++++++++++++++++++ 2 files changed, 145 insertions(+), 13 deletions(-) diff --git a/packages/uipath/src/uipath/eval/evaluators/evaluator_factory.py b/packages/uipath/src/uipath/eval/evaluators/evaluator_factory.py index 6a80caac6..f37750533 100644 --- a/packages/uipath/src/uipath/eval/evaluators/evaluator_factory.py +++ b/packages/uipath/src/uipath/eval/evaluators/evaluator_factory.py @@ -1,5 +1,6 @@ """Factory class for creating evaluator instances based on configuration.""" +import hashlib import importlib.util import logging import sys @@ -146,19 +147,34 @@ def _create_custom_coded_evaluator_internal( f"Make sure the file exists in the evaluators/custom/ directory" ) - module_name = f"_custom_evaluator_{file_path.stem}_{id(data)}" - spec = importlib.util.spec_from_file_location(module_name, file_path) - if spec is None or spec.loader is None: - raise ValueError(f"Could not load module from {file_path}") - - module = importlib.util.module_from_spec(spec) - sys.modules[module_name] = module - try: - spec.loader.exec_module(module) - except Exception as e: - raise ValueError( - f"Error executing module from {file_path}: {str(e)}" - ) from e + # Cache the module under a name derived from its resolved path so that + # sys.modules acts as the cache: repeated creations reuse the already + # loaded module instead of re-exec()'ing it. The name previously embedded + # id(data) (a fresh dict, so a unique value on every call), which defeated + # the cache and leaked a new module into sys.modules on each call — + # unbounded growth (and O(n^2) cost) when evaluators are built per + # datapoint. + resolved_path = file_path.resolve() + module_name = ( + f"_custom_evaluator_{resolved_path.stem}_" + f"{hashlib.sha256(str(resolved_path).encode()).hexdigest()[:16]}" + ) + module = sys.modules.get(module_name) + if module is None: + spec = importlib.util.spec_from_file_location(module_name, resolved_path) + if spec is None or spec.loader is None: + raise ValueError(f"Could not load module from {file_path}") + + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + try: + spec.loader.exec_module(module) + except Exception as e: + # Don't leave a half-initialized module cached under this name. + sys.modules.pop(module_name, None) + raise ValueError( + f"Error executing module from {file_path}: {str(e)}" + ) from e # Get the class from the module if not hasattr(module, class_name): diff --git a/packages/uipath/tests/evaluators/test_evaluator_factory.py b/packages/uipath/tests/evaluators/test_evaluator_factory.py index 0e44c4697..b180867d9 100644 --- a/packages/uipath/tests/evaluators/test_evaluator_factory.py +++ b/packages/uipath/tests/evaluators/test_evaluator_factory.py @@ -7,6 +7,8 @@ - Proper instantiation across all evaluator types """ +import sys +from pathlib import Path from typing import Any import pytest @@ -448,3 +450,117 @@ def test_property_setters_work(self) -> None: assert evaluator.name == "UpdatedName" assert evaluator.description == "Updated description" + + +class TestCustomCodedEvaluatorModuleLoading: + """Custom-coded evaluators must load their module once, without leaking sys.modules.""" + + @staticmethod + def _write_evaluator_module(path: Path) -> None: + path.write_text( + "from uipath.eval.evaluators.exact_match_evaluator import ExactMatchEvaluator\n" + "\n" + "class MyCustomEvaluator(ExactMatchEvaluator):\n" + " pass\n" + ) + + @staticmethod + def _config(module_path: Path) -> dict[str, Any]: + return { + "version": "1.0", + "id": "TestCustom", + "evaluatorTypeId": "uipath-exact-match", + "evaluatorSchema": f"file://{module_path}:MyCustomEvaluator", + "evaluatorConfig": { + "targetOutputKey": "*", + "negated": False, + "ignoreCase": False, + }, + } + + def test_module_is_cached_and_not_leaked(self, tmp_path: Path) -> None: + """Repeated creation from the same file reuses one cached module. + + Regression test: the module name previously embedded ``id(data)`` (a fresh + dict per call), so every ``create_evaluator`` call re-executed the module + and leaked a new entry into ``sys.modules`` — unbounded growth (and O(n^2) + cost) when evaluators are built per datapoint. The module must now load + once and be reused. + """ + module_path = tmp_path / "my_custom_eval.py" + self._write_evaluator_module(module_path) + + def custom_module_keys() -> set[str]: + return {k for k in sys.modules if k.startswith("_custom_evaluator_")} + + before = custom_module_keys() + try: + first = EvaluatorFactory.create_evaluator(self._config(module_path)) + after_first = custom_module_keys() + second = EvaluatorFactory.create_evaluator(self._config(module_path)) + third = EvaluatorFactory.create_evaluator(self._config(module_path)) + after_third = custom_module_keys() + + # Same class object across calls => the module was loaded once and reused. + assert type(first) is type(second) is type(third) + # Exactly one module added, and no further growth on subsequent calls. + assert len(after_first) == len(before) + 1 + assert after_third == after_first + finally: + for key in custom_module_keys() - before: + del sys.modules[key] + + def test_failed_load_is_not_cached(self, tmp_path: Path) -> None: + """A module that raises during import must not be left cached. + + The exec-failure branch pops the half-initialized module from sys.modules + so a fixed file can be retried in the same process; without it the broken + load would be sticky (the path-keyed cache short-circuits the retry). + """ + module_path = tmp_path / "broken_eval.py" + module_path.write_text("raise RuntimeError('boom at import time')\n") + + def custom_module_keys() -> set[str]: + return {k for k in sys.modules if k.startswith("_custom_evaluator_")} + + before = custom_module_keys() + try: + with pytest.raises(ValueError): + EvaluatorFactory.create_evaluator(self._config(module_path)) + # The failed load left nothing cached, so the same path can be retried. + assert custom_module_keys() == before + + # Fix the file; a subsequent create for the same path must now succeed. + self._write_evaluator_module(module_path) + evaluator = EvaluatorFactory.create_evaluator(self._config(module_path)) + assert type(evaluator).__name__ == "MyCustomEvaluator" + finally: + for key in custom_module_keys() - before: + del sys.modules[key] + + def test_same_stem_in_different_dirs_do_not_collide(self, tmp_path: Path) -> None: + """Two custom-evaluator files sharing a basename load as distinct modules. + + The path hash in the module name is what disambiguates them; a stem-only + key would return the first file's class for both. + """ + path_a = tmp_path / "a" / "my_eval.py" + path_b = tmp_path / "b" / "my_eval.py" + path_a.parent.mkdir() + path_b.parent.mkdir() + self._write_evaluator_module(path_a) + self._write_evaluator_module(path_b) + + def custom_module_keys() -> set[str]: + return {k for k in sys.modules if k.startswith("_custom_evaluator_")} + + before = custom_module_keys() + try: + eval_a = EvaluatorFactory.create_evaluator(self._config(path_a)) + eval_b = EvaluatorFactory.create_evaluator(self._config(path_b)) + # Distinct files => distinct cached modules => distinct class objects. + assert type(eval_a) is not type(eval_b) + assert len(custom_module_keys() - before) == 2 + finally: + for key in custom_module_keys() - before: + del sys.modules[key]