Skip to content

Commit d7835b8

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). Also harden the loader: the evaluatorSchema path is evaluator config, so reject '..' path-traversal segments before resolving/loading it, and report the resolved path in the load error messages. Regression tests cover: repeated creation reuses one module (no leak), a failed load is not cached, two files sharing a basename load as distinct modules, and path-traversal segments are rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 487bf03 commit d7835b8

2 files changed

Lines changed: 170 additions & 13 deletions

File tree

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

Lines changed: 37 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
@@ -122,6 +123,14 @@ def _create_custom_coded_evaluator_internal(
122123
ValueError: If file or class cannot be loaded, or if the class is not a BaseEvaluator subclass
123124
"""
124125
file_path = Path(file_path_str)
126+
# The schema path comes from evaluator configuration, so treat it as
127+
# untrusted input: reject path-traversal ('..') segments before the path
128+
# is resolved and handed to the module loader, so a config value cannot
129+
# walk out of its intended location to load an arbitrary file.
130+
if ".." in file_path.parts:
131+
raise ValueError(
132+
f"Custom evaluator path must not contain '..' segments: {file_path_str!r}"
133+
)
125134
if not file_path.is_absolute():
126135
if not file_path.exists():
127136
if evaluators_dir is not None:
@@ -146,19 +155,34 @@ def _create_custom_coded_evaluator_internal(
146155
f"Make sure the file exists in the evaluators/custom/ directory"
147156
)
148157

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

163187
# Get the class from the module
164188
if not hasattr(module, class_name):

packages/uipath/tests/evaluators/test_evaluator_factory.py

Lines changed: 133 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,134 @@ 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+
config = self._config(module_path)
528+
try:
529+
with pytest.raises(ValueError):
530+
EvaluatorFactory.create_evaluator(config)
531+
# The failed load left nothing cached, so the same path can be retried.
532+
assert custom_module_keys() == before
533+
534+
# Fix the file; a subsequent create for the same path must now succeed.
535+
self._write_evaluator_module(module_path)
536+
evaluator = EvaluatorFactory.create_evaluator(self._config(module_path))
537+
assert type(evaluator).__name__ == "MyCustomEvaluator"
538+
finally:
539+
for key in custom_module_keys() - before:
540+
del sys.modules[key]
541+
542+
def test_same_stem_in_different_dirs_do_not_collide(self, tmp_path: Path) -> None:
543+
"""Two custom-evaluator files sharing a basename load as distinct modules.
544+
545+
The path hash in the module name is what disambiguates them; a stem-only
546+
key would return the first file's class for both.
547+
"""
548+
path_a = tmp_path / "a" / "my_eval.py"
549+
path_b = tmp_path / "b" / "my_eval.py"
550+
path_a.parent.mkdir()
551+
path_b.parent.mkdir()
552+
self._write_evaluator_module(path_a)
553+
self._write_evaluator_module(path_b)
554+
555+
def custom_module_keys() -> set[str]:
556+
return {k for k in sys.modules if k.startswith("_custom_evaluator_")}
557+
558+
before = custom_module_keys()
559+
try:
560+
eval_a = EvaluatorFactory.create_evaluator(self._config(path_a))
561+
eval_b = EvaluatorFactory.create_evaluator(self._config(path_b))
562+
# Distinct files => distinct cached modules => distinct class objects.
563+
assert type(eval_a) is not type(eval_b)
564+
assert len(custom_module_keys() - before) == 2
565+
finally:
566+
for key in custom_module_keys() - before:
567+
del sys.modules[key]
568+
569+
def test_path_traversal_is_rejected(self) -> None:
570+
"""A schema path with '..' segments is refused before any file load."""
571+
config = {
572+
"version": "1.0",
573+
"id": "TestTraversal",
574+
"evaluatorTypeId": "uipath-exact-match",
575+
"evaluatorSchema": "file://../../../etc/passwd:MyCustomEvaluator",
576+
"evaluatorConfig": {
577+
"targetOutputKey": "*",
578+
"negated": False,
579+
"ignoreCase": False,
580+
},
581+
}
582+
with pytest.raises(ValueError, match="must not contain"):
583+
EvaluatorFactory.create_evaluator(config)

0 commit comments

Comments
 (0)