Skip to content

Commit 6021f5e

Browse files
committed
refactor(evaluator): trim comments and extract the run session
Address review: the run-level cache is not specific to structured output, so it moves to its own module as begin_evaluation_session(). Extract the agent-eval preflight sweep into a function, hoist a function-local import, and cut comments and docstrings that narrated the edits rather than the code. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
1 parent 144033a commit 6021f5e

13 files changed

Lines changed: 234 additions & 410 deletions

File tree

packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.py

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@
5151
resolve_target_structured_output_mode,
5252
run_sync,
5353
)
54-
from nemo_evaluator_sdk.structured_output import structured_output_mode_session
54+
from nemo_evaluator_sdk.session import begin_evaluation_session
5555
from nemo_evaluator_sdk.execution.samples import build_metric_input
5656
from nemo_evaluator_sdk.inference import InferenceFn
5757
from nemo_evaluator_sdk.metrics.protocol import Metric, MetricWithPreflight, validate_metric_result
@@ -166,10 +166,7 @@ async def run(
166166
runtime_config = resolved_config.model_copy(update={"run_id": run_id})
167167
started_at = datetime.now(UTC)
168168

169-
# One detection session for the whole run: generation probes the target and scoring probes
170-
# any judge model, and imported-trial runs still score, so scoping this to generation alone
171-
# would leave judges probing per call.
172-
async with structured_output_mode_session():
169+
async with begin_evaluation_session():
173170
# Branch on which seam was supplied so the type checker can narrow ``target`` to a
174171
# concrete ``AgentEvalTarget`` without a cast.
175172
if trials is not None:
@@ -248,18 +245,7 @@ async def _score_trials(
248245
if not task.metrics:
249246
raise ValueError(f"task {task.id!r} does not declare any metrics")
250247

251-
# Agent-eval scores metrics directly rather than through prepare_metric_for_execution, so
252-
# nothing else runs their preflight. An LLM judge detects its endpoint's structured-output
253-
# encoding there; without this it would score using the provisional guess from new_hooks.
254-
# Deduplicated by identity because the same metric object is scored once per trial. The run
255-
# session would collapse repeat probes to one request anyway; this just avoids the repeated
256-
# awaits. Identity is stable here: `tasks` holds every metric for the duration of the loop.
257-
preflighted: set[int] = set()
258-
for task in tasks:
259-
for metric in task.metrics:
260-
if isinstance(metric, MetricWithPreflight) and id(metric) not in preflighted:
261-
preflighted.add(id(metric))
262-
await metric.preflight()
248+
await _preflight_task_metrics(tasks)
263249

264250
semaphore = asyncio.Semaphore(config.parallelism)
265251

@@ -333,8 +319,6 @@ async def _generate_trials(
333319
params = _resolve_live_params(config, target)
334320
prompt_template = config.prompt_template or _default_prompt_template(target)
335321
semaphore = asyncio.Semaphore(params.parallelism)
336-
# Hooks are built per row below; the run-level session opened by run() is what keeps the
337-
# endpoint probe to one round trip for the whole pass instead of one per row.
338322

339323
# Use the injected transport client when provided; otherwise build a default for the
340324
# resolved target type and close it when generation finishes.
@@ -402,6 +386,20 @@ async def generate_one(index: int, task: AgentEvalTask) -> AgentEvalTrial:
402386
await close_client()
403387

404388

389+
async def _preflight_task_metrics(tasks: Sequence[AgentEvalTask]) -> None:
390+
"""Run each distinct metric's preflight once.
391+
392+
Agent-eval scores metrics directly rather than through ``prepare_metric_for_execution``, so
393+
nothing else runs their preflight.
394+
"""
395+
preflighted: set[int] = set()
396+
for task in tasks:
397+
for metric in task.metrics:
398+
if isinstance(metric, MetricWithPreflight) and id(metric) not in preflighted:
399+
preflighted.add(id(metric))
400+
await metric.preflight()
401+
402+
405403
async def _generate_sample(
406404
*,
407405
target: Model | Agent,
@@ -423,8 +421,6 @@ async def _generate_sample(
423421
model_inference_fn = (
424422
cast(InferenceFn, inference_fn) if inference_fn is not None else inference.make_inference_request
425423
)
426-
# Hooks are built per row here, so this relies on detection being cached per endpoint:
427-
# without the probe the request would carry whichever encoding new_hooks guessed.
428424
await resolve_target_structured_output_mode(
429425
preprocess_hooks=preprocess_hooks,
430426
model=target,

packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.py

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
from nemo_evaluator_sdk.inference import PostprocessResponse, PreprocessRequest
2525
from nemo_evaluator_sdk.metrics.protocol import Metric
2626
from nemo_evaluator_sdk.resolvers import LocalModelResolver, LocalSecretResolver
27-
from nemo_evaluator_sdk.structured_output import structured_output_mode_session
27+
from nemo_evaluator_sdk.session import begin_evaluation_session
2828
from nemo_evaluator_sdk.values import Agent, DatasetInput, FieldMapping, Model
2929
from nemo_evaluator_sdk.values.multi_metric_results import BenchmarkEvaluationResult, namespace_result
3030
from nemo_evaluator_sdk.values.results import AggregateFieldName, EvaluationResult
@@ -85,10 +85,8 @@ async def _evaluate_one(
8585
Returns:
8686
A namespaced single-metric evaluation result.
8787
"""
88-
# Wraps preparation as well as execution: prepare_metric_for_execution runs the metric's
89-
# preflight, which is where an LLM judge probes its endpoint. evaluate_metric opens a
90-
# session too; the session is re-entrant, so both share this one cache.
91-
async with structured_output_mode_session():
88+
# Preparation runs each metric's preflight, so it belongs inside the session.
89+
async with begin_evaluation_session():
9290
return await self._evaluate_one_in_session(
9391
metric=metric,
9492
metric_key=metric_key,
@@ -114,7 +112,6 @@ async def _evaluate_one_in_session(
114112
postprocess_hooks: tuple[PostprocessResponse, ...] | None,
115113
rows: list[dict[str, Any]],
116114
) -> EvaluationResult:
117-
"""Body of :meth:`_evaluate_one`, inside a structured-output detection session."""
118115
prepared_metric = await prepare_metric_for_execution(
119116
metric,
120117
params=params,
@@ -168,10 +165,8 @@ async def evaluate_dataset(
168165
"""
169166
rows = _prepare_rows(dataset, params, field_mapping)
170167
metric_keys = unique_metric_keys(metrics)
171-
# Opened before metric preparation on purpose: prepare_metric_for_execution runs each
172-
# metric's preflight, which is where an LLM judge probes its endpoint. Starting the session
173-
# later would leave those probes uncached, so two judges on one endpoint would each probe.
174-
async with structured_output_mode_session():
168+
# Preparation runs each metric's preflight, so it belongs inside the session.
169+
async with begin_evaluation_session():
175170
return await self._evaluate_dataset_in_session(
176171
metrics=metrics,
177172
metric_keys=metric_keys,
@@ -197,7 +192,6 @@ async def _evaluate_dataset_in_session(
197192
preprocess_hooks: Sequence[PreprocessRequest] | None,
198193
postprocess_hooks: Sequence[PostprocessResponse] | None,
199194
) -> BenchmarkEvaluationResult:
200-
"""Body of :meth:`evaluate_dataset`, inside a structured-output detection session."""
201195
prepared_metrics = [
202196
await prepare_metric_for_execution(
203197
metric,
@@ -218,16 +212,13 @@ async def _evaluate_dataset_in_session(
218212
else:
219213
merged_preprocess_hooks = tuple(preprocess_hooks or ())
220214
merged_postprocess_hooks = tuple(postprocess_hooks or ())
221-
# This multi-metric path builds target hooks itself and calls evaluate_benchmark directly,
222-
# bypassing evaluate_metric, so it has to resolve the hook here or generation would send
223-
# whichever encoding new_hooks guessed.
215+
# This path builds target hooks itself and bypasses evaluate_metric, so it resolves here.
224216
if isinstance(target, Model):
225217
await resolve_target_structured_output_mode(
226218
preprocess_hooks=merged_preprocess_hooks,
227219
model=target,
228-
# Resolved from the benchmark module at call time, not bound here, so the probe
229-
# always travels the exact transport that evaluate_benchmark's own default uses --
230-
# including when that binding is swapped.
220+
# Attribute lookup, not a bound import: the probe must use the same transport
221+
# evaluate_benchmark defaults to, including when that binding is swapped.
231222
inference_fn=benchmark_execution.make_inference_request,
232223
params=params,
233224
)

packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/metric_execution.py

Lines changed: 11 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,11 @@
4848
from nemo_evaluator_sdk.metrics.utils import metric_type_name
4949
from nemo_evaluator_sdk.resilience.api import run_indexed_tasks, use_resilience_session
5050
from nemo_evaluator_sdk.resilience.errors import get_evaluation_error
51-
from nemo_evaluator_sdk.structured_output import structured_output_mode_session
51+
from nemo_evaluator_sdk.session import begin_evaluation_session
52+
from nemo_evaluator_sdk.structured_output import (
53+
InferenceStructuredOutput,
54+
detect_structured_output_mode,
55+
)
5256
from nemo_evaluator_sdk.templates import render_request
5357
from nemo_evaluator_sdk.values import (
5458
Agent,
@@ -265,25 +269,11 @@ async def resolve_target_structured_output_mode(
265269
inference_fn: inference.InferenceFn,
266270
params: RunConfig | RunConfigOnline | RunConfigOnlineModel | None,
267271
) -> None:
268-
"""Probe the target endpoint so generation uses an encoding it actually honours.
269-
270-
The judge has its own preflight; target generation had none, and previously relied on the
271-
model's ``format`` label to pick an encoding. That label is deprecated and ignored, so without
272-
this probe a target that only accepts ``nvext.guided_json`` would be sent ``response_format``
273-
and either reject the request or silently drop the constraint.
274-
275-
Only the hook built from ``params.structured_output`` is retuned. A caller that hand-builds an
276-
InferenceStructuredOutput and splices it in via ``preprocess_hooks`` has chosen a mode
277-
deliberately, and silently overriding that choice would be surprising; ``_merge_online_hooks``
278-
places the run-level hook ahead of caller-supplied ones, so the first match is ours.
279-
280-
Like the judge's own preflight, this runs before ``use_resilience_session()`` opens, so probe
281-
attempts use the process-global scheduler and do not appear in the session summary. Detection
282-
itself is cached per endpoint for the run, so a Model used as both target and judge is probed
283-
once, not once per role.
284-
"""
285-
from nemo_evaluator_sdk.structured_output import InferenceStructuredOutput, detect_structured_output_mode
272+
"""Probe the target endpoint and set the generation hook's encoding.
286273
274+
Only the hook built from ``params.structured_output`` is retuned; a caller that supplies its own
275+
InferenceStructuredOutput has chosen a mode deliberately.
276+
"""
287277
if not (isinstance(params, RunConfigOnlineModel) and params.structured_output):
288278
return
289279

@@ -308,13 +298,7 @@ def _maybe_set_default_max_tokens(
308298
request: dict[str, Any],
309299
params: RunConfigOnlineModel | None,
310300
) -> None:
311-
"""Apply the default max token cap when neither params nor request set one.
312-
313-
This was previously applied only to ``nim``-format models. Model format is deprecated and no
314-
longer distinguishes endpoints, so the cap is unconditional: an unbounded generation against a
315-
reasoning-capable judge is a cost risk on any endpoint, and callers that want more can set
316-
``max_tokens`` or ``max_completion_tokens`` explicitly.
317-
"""
301+
"""Apply the default max token cap when neither params nor request set one."""
318302
inference_params = params.inference if isinstance(params, RunConfigOnlineModel) else None
319303
if inference_params is not None and (
320304
inference_params.max_tokens is not None or inference_params.max_completion_tokens is not None
@@ -868,9 +852,7 @@ async def evaluate_metric(
868852
log.warning("No rows found in dataset, returning empty evaluation result")
869853
return empty_evaluation_result()
870854

871-
# Scope endpoint structured-output detection to this run so probes are cached across rows
872-
# without leaking a result (or a transient failure) into the next run.
873-
async with structured_output_mode_session():
855+
async with begin_evaluation_session():
874856
return await _evaluate_metric_in_session(
875857
metric=metric,
876858
rows=rows,
@@ -892,7 +874,6 @@ async def _evaluate_metric_in_session(
892874
preprocess_hooks: Sequence[inference.PreprocessRequest] | None,
893875
postprocess_hooks: Sequence[inference.PostprocessResponse] | None,
894876
) -> EvaluationResult:
895-
"""Body of :func:`evaluate_metric`, run inside a structured-output detection session."""
896877
params = resolve_params(params, target)
897878

898879
client_close_fn = None

packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/llm_judge.py

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -200,14 +200,8 @@ async def resolve_models(self, model_resolver: ModelResolver) -> None:
200200
await resolve_model_refs(self, model_resolver)
201201
self._client = None
202202
self._api_key = None
203-
# `_preflight_lock` is deliberately NOT reset here. Clearing it would split preflight
204-
# synchronisation while a concurrent compute_scores() is mid-probe: a second scorer would
205-
# build a new lock, and an in-flight scorer could then render through the freshly rebuilt
206-
# unresolved hook and send a guessed encoding with only a warning. A lock retained from a
207-
# previous event loop instead fails loudly on its next contended acquire, and both cases
208-
# require resolving models while scoring with the same metric object, which is unsupported:
209-
# resolution is preparation-time, before evaluation starts. A loud failure beats a silent
210-
# unprobed request in a change whose whole purpose is removing silent degradation.
203+
# `_preflight_lock` is deliberately not reset: clearing it mid-run would split preflight
204+
# synchronisation and let an in-flight scorer send an unprobed request.
211205
self._ensure_default_prompt_template()
212206
preprocess_hooks, postprocess_hooks = new_hooks(self)
213207
self.with_hooks(preprocess=preprocess_hooks, postprocess=postprocess_hooks)
@@ -250,11 +244,7 @@ async def preflight(self) -> None:
250244
_logger.info("Structured output mode selected for %s: %s", model.name, mode.value)
251245

252246
def _require_preflight_lock(self) -> asyncio.Lock:
253-
"""Return this metric's preflight lock, created on first use.
254-
255-
Built lazily rather than as a field default so the metric stays deep-copyable and does not
256-
bind a lock to an event loop at construction time.
257-
"""
247+
"""Return this metric's preflight lock, created lazily to keep the metric deep-copyable."""
258248
if self._preflight_lock is None:
259249
self._preflight_lock = asyncio.Lock()
260250
return self._preflight_lock
@@ -357,13 +347,8 @@ def _retry_with_max_completion_tokens(self, request: dict) -> dict:
357347

358348
async def compute_scores(self, input: MetricInput) -> MetricResult:
359349
"""Compute structured score output for one item/sample pair."""
360-
# Executors call preflight() before scoring, but this is public API: a caller can build the
361-
# metric and score with it directly, and then nothing would have probed the endpoint. Detect
362-
# on first use so the encoding is never a guess.
363-
#
364-
# Locked and re-checked because a direct caller may gather many compute_scores() at once
365-
# with no run session to cache detection: without this, every one of them would see an
366-
# unresolved hook and probe the same endpoint concurrently.
350+
# Public API: a caller may score without an executor having run preflight. Locked because
351+
# concurrent direct callers would otherwise each probe the same endpoint.
367352
if self._has_unresolved_structured_output_hook():
368353
async with self._require_preflight_lock():
369354
if self._has_unresolved_structured_output_hook():
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
# SPDX-License-Identifier: Apache-2.0
3+
4+
"""Run-scoped cache shared by everything participating in one evaluation."""
5+
6+
import asyncio
7+
from collections.abc import AsyncIterator, Hashable
8+
from contextlib import asynccontextmanager
9+
from contextvars import ContextVar
10+
from typing import Any
11+
12+
_CACHE: ContextVar[dict[Hashable, Any] | None] = ContextVar("evaluation_session_cache", default=None)
13+
_LOCK: ContextVar[asyncio.Lock | None] = ContextVar("evaluation_session_lock", default=None)
14+
15+
16+
@asynccontextmanager
17+
async def begin_evaluation_session() -> AsyncIterator[None]:
18+
"""Scope cached evaluation state to one run.
19+
20+
Entries live only inside this boundary, which is what makes caching a failed result safe: a
21+
transient failure is retried by the next run instead of persisting for the life of the process.
22+
Outside a session nothing is cached and every caller recomputes.
23+
24+
Re-entrant. Entry points nest -- a backend opens a session, then the metric executor opens
25+
another -- and a fresh inner cache would recompute what the run already resolved.
26+
"""
27+
if _CACHE.get() is not None:
28+
yield
29+
return
30+
31+
cache_token = _CACHE.set({})
32+
lock_token = _LOCK.set(asyncio.Lock())
33+
try:
34+
yield
35+
finally:
36+
_CACHE.reset(cache_token)
37+
_LOCK.reset(lock_token)
38+
39+
40+
def session_cache() -> dict[Hashable, Any] | None:
41+
"""Return the active run's cache, or None outside a session."""
42+
return _CACHE.get()
43+
44+
45+
def session_lock() -> asyncio.Lock:
46+
"""Return the active run's lock, or a throwaway one outside a session."""
47+
return _LOCK.get() or asyncio.Lock()

0 commit comments

Comments
 (0)