Skip to content

Commit 12090e5

Browse files
adrian-badeaclaude
andcommitted
fix(eval): cache custom-coded evaluator modules instead of leaking sys.modules
_create_custom_coded_evaluator_internal named the dynamically loaded module `_custom_evaluator_<stem>_<id(data)>`. 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 <noreply@anthropic.com>
1 parent 487bf03 commit 12090e5

2 files changed

Lines changed: 145 additions & 13 deletions

File tree

packages/uipath/src/uipath/eval/evaluators/evaluator_factory.py

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
"""Factory class for creating evaluator instances based on configuration."""
22

3+
import hashlib
34
import importlib.util
45
import logging
56
import sys
@@ -146,19 +147,34 @@ def _create_custom_coded_evaluator_internal(
146147
f"Make sure the file exists in the evaluators/custom/ directory"
147148
)
148149

149-
module_name = f"_custom_evaluator_{file_path.stem}_{id(data)}"
150-
spec = importlib.util.spec_from_file_location(module_name, file_path)
151-
if spec is None or spec.loader is None:
152-
raise ValueError(f"Could not load module from {file_path}")
153-
154-
module = importlib.util.module_from_spec(spec)
155-
sys.modules[module_name] = module
156-
try:
157-
spec.loader.exec_module(module)
158-
except Exception as e:
159-
raise ValueError(
160-
f"Error executing module from {file_path}: {str(e)}"
161-
) from e
150+
# Cache the module under a name derived from its resolved path so that
151+
# sys.modules acts as the cache: repeated creations reuse the already
152+
# loaded module instead of re-exec()'ing it. The name previously embedded
153+
# id(data) (a fresh dict, so a unique value on every call), which defeated
154+
# the cache and leaked a new module into sys.modules on each call —
155+
# unbounded growth (and O(n^2) cost) when evaluators are built per
156+
# datapoint.
157+
resolved_path = file_path.resolve()
158+
module_name = (
159+
f"_custom_evaluator_{resolved_path.stem}_"
160+
f"{hashlib.sha256(str(resolved_path).encode()).hexdigest()[:16]}"
161+
)
162+
module = sys.modules.get(module_name)
163+
if module is None:
164+
spec = importlib.util.spec_from_file_location(module_name, resolved_path)
165+
if spec is None or spec.loader is None:
166+
raise ValueError(f"Could not load module from {file_path}")
167+
168+
module = importlib.util.module_from_spec(spec)
169+
sys.modules[module_name] = module
170+
try:
171+
spec.loader.exec_module(module)
172+
except Exception as e:
173+
# Don't leave a half-initialized module cached under this name.
174+
sys.modules.pop(module_name, None)
175+
raise ValueError(
176+
f"Error executing module from {file_path}: {str(e)}"
177+
) from e
162178

163179
# Get the class from the module
164180
if not hasattr(module, class_name):

packages/uipath/tests/evaluators/test_evaluator_factory.py

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
- Proper instantiation across all evaluator types
88
"""
99

10+
import sys
11+
from pathlib import Path
1012
from typing import Any
1113

1214
import pytest
@@ -448,3 +450,117 @@ def test_property_setters_work(self) -> None:
448450

449451
assert evaluator.name == "UpdatedName"
450452
assert evaluator.description == "Updated description"
453+
454+
455+
class TestCustomCodedEvaluatorModuleLoading:
456+
"""Custom-coded evaluators must load their module once, without leaking sys.modules."""
457+
458+
@staticmethod
459+
def _write_evaluator_module(path: Path) -> None:
460+
path.write_text(
461+
"from uipath.eval.evaluators.exact_match_evaluator import ExactMatchEvaluator\n"
462+
"\n"
463+
"class MyCustomEvaluator(ExactMatchEvaluator):\n"
464+
" pass\n"
465+
)
466+
467+
@staticmethod
468+
def _config(module_path: Path) -> dict[str, Any]:
469+
return {
470+
"version": "1.0",
471+
"id": "TestCustom",
472+
"evaluatorTypeId": "uipath-exact-match",
473+
"evaluatorSchema": f"file://{module_path}:MyCustomEvaluator",
474+
"evaluatorConfig": {
475+
"targetOutputKey": "*",
476+
"negated": False,
477+
"ignoreCase": False,
478+
},
479+
}
480+
481+
def test_module_is_cached_and_not_leaked(self, tmp_path: Path) -> None:
482+
"""Repeated creation from the same file reuses one cached module.
483+
484+
Regression test: the module name previously embedded ``id(data)`` (a fresh
485+
dict per call), so every ``create_evaluator`` call re-executed the module
486+
and leaked a new entry into ``sys.modules`` — unbounded growth (and O(n^2)
487+
cost) when evaluators are built per datapoint. The module must now load
488+
once and be reused.
489+
"""
490+
module_path = tmp_path / "my_custom_eval.py"
491+
self._write_evaluator_module(module_path)
492+
493+
def custom_module_keys() -> set[str]:
494+
return {k for k in sys.modules if k.startswith("_custom_evaluator_")}
495+
496+
before = custom_module_keys()
497+
try:
498+
first = EvaluatorFactory.create_evaluator(self._config(module_path))
499+
after_first = custom_module_keys()
500+
second = EvaluatorFactory.create_evaluator(self._config(module_path))
501+
third = EvaluatorFactory.create_evaluator(self._config(module_path))
502+
after_third = custom_module_keys()
503+
504+
# Same class object across calls => the module was loaded once and reused.
505+
assert type(first) is type(second) is type(third)
506+
# Exactly one module added, and no further growth on subsequent calls.
507+
assert len(after_first) == len(before) + 1
508+
assert after_third == after_first
509+
finally:
510+
for key in custom_module_keys() - before:
511+
del sys.modules[key]
512+
513+
def test_failed_load_is_not_cached(self, tmp_path: Path) -> None:
514+
"""A module that raises during import must not be left cached.
515+
516+
The exec-failure branch pops the half-initialized module from sys.modules
517+
so a fixed file can be retried in the same process; without it the broken
518+
load would be sticky (the path-keyed cache short-circuits the retry).
519+
"""
520+
module_path = tmp_path / "broken_eval.py"
521+
module_path.write_text("raise RuntimeError('boom at import time')\n")
522+
523+
def custom_module_keys() -> set[str]:
524+
return {k for k in sys.modules if k.startswith("_custom_evaluator_")}
525+
526+
before = custom_module_keys()
527+
try:
528+
with pytest.raises(ValueError):
529+
EvaluatorFactory.create_evaluator(self._config(module_path))
530+
# The failed load left nothing cached, so the same path can be retried.
531+
assert custom_module_keys() == before
532+
533+
# Fix the file; a subsequent create for the same path must now succeed.
534+
self._write_evaluator_module(module_path)
535+
evaluator = EvaluatorFactory.create_evaluator(self._config(module_path))
536+
assert type(evaluator).__name__ == "MyCustomEvaluator"
537+
finally:
538+
for key in custom_module_keys() - before:
539+
del sys.modules[key]
540+
541+
def test_same_stem_in_different_dirs_do_not_collide(self, tmp_path: Path) -> None:
542+
"""Two custom-evaluator files sharing a basename load as distinct modules.
543+
544+
The path hash in the module name is what disambiguates them; a stem-only
545+
key would return the first file's class for both.
546+
"""
547+
path_a = tmp_path / "a" / "my_eval.py"
548+
path_b = tmp_path / "b" / "my_eval.py"
549+
path_a.parent.mkdir()
550+
path_b.parent.mkdir()
551+
self._write_evaluator_module(path_a)
552+
self._write_evaluator_module(path_b)
553+
554+
def custom_module_keys() -> set[str]:
555+
return {k for k in sys.modules if k.startswith("_custom_evaluator_")}
556+
557+
before = custom_module_keys()
558+
try:
559+
eval_a = EvaluatorFactory.create_evaluator(self._config(path_a))
560+
eval_b = EvaluatorFactory.create_evaluator(self._config(path_b))
561+
# Distinct files => distinct cached modules => distinct class objects.
562+
assert type(eval_a) is not type(eval_b)
563+
assert len(custom_module_keys() - before) == 2
564+
finally:
565+
for key in custom_module_keys() - before:
566+
del sys.modules[key]

0 commit comments

Comments
 (0)