fix(evals): reject non-positive OnlineEvaluator.max_concurrency#6267
Conversation
📝 WalkthroughWalkthrough
Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
David's AICA here: 🔍 Local review suite (automated, pre-merge)
Verdict: ✅ all 3 waves passed, 0 blocking, 0 required
|
Match the '>= 1, got N' phrasing used by OnlineEvaluator (pydantic#6267), note the constraint in the ConcurrencyLimit and ConcurrencyLimiter docstrings, and drop the stale 'before a run can hang' test wording.
OnlineEvaluatorshould reject non-positivemax_concurrency#6266This rejects invalid non-positive
OnlineEvaluator.max_concurrencyvalues before they create a semaphore that cannot run evaluations.What Problem This Solves
OnlineEvaluator.max_concurrencyis a user-facing configuration value for bounding how many online evaluations may run at the same time. A positive value has a clear meaning:1allows one in-flight evaluation,2allows two, and so on.0, however, does not provide a useful concurrency budget for this API.Today,
OnlineEvaluator.__post_init__passes the value directly intothreading.Semaphore(self.max_concurrency). That creates two different failure modes for the same class of invalid input:max_concurrency=-1fails immediately with Python's low-levelValueError: semaphore initial value must be >= 0.max_concurrency=0is accepted becausethreading.Semaphore(0)is valid, but it starts with zero available permits.The second case is the confusing one. Once the evaluator is attached, the online evaluation dispatch path checks capacity with:
For a zero-permit semaphore, that call always returns
False. As a result, every sampled evaluation for thatOnlineEvaluatoris treated as if the evaluator is already at its concurrency limit. The run does not fail at configuration time; it simply follows the documented ?limit reached? path.That makes the bug easy to miss in real usage. If an
on_max_concurrencycallback is configured, users may see every evaluation reported as dropped even though there is no real concurrency pressure. If no callback is configured, those evaluations are silently ignored, which can make online evaluation appear to be installed while producing no results.This is different from intentionally disabling evaluation. The API already has an explicit sampling control for that:
sample_rate=0.0. By contrast,max_concurrencyshould describe the positive number of evaluations that may run concurrently after an evaluation has been sampled.This change also aligns
OnlineEvaluatorwith the existing validation inDataset.evaluate(..., max_concurrency<=0), which already rejects non-positive concurrency limits before constructing a semaphore.Change
This change adds explicit validation for
OnlineEvaluator.max_concurrencyinsideOnlineEvaluator.__post_init__, before the class creates its internalthreading.Semaphore.That placement is intentional.
__post_init__is the point where the dataclass turns user-supplied configuration into runtime state, so it is the narrowest place to reject invalid concurrency settings before they become background-dispatch behavior. A positive value still means the same thing as before: the number of online evaluations that may run at the same time. Values below1now fail fast because they do not represent a useful concurrency limit for this API.The new guard is intentionally small:
This gives the two invalid boundaries the same user-facing treatment:
max_concurrency=0now raises immediately instead of creating a zero-permit semaphore. Without the guard, every lateracquire(blocking=False)call fails and every sampled online evaluation looks like it hit a real concurrency limit.max_concurrency=-1now raises the sameOnlineEvaluatorvalidation error instead of leaking Python's lower-levelthreading.Semaphorevalidation message.Positive concurrency limits are unchanged. The default
10, the single-worker case1, and higher limits still create the same semaphore-backed limiter. The existingon_max_concurrencypath is still used when a valid positive limit is reached by real concurrent demand.The test update covers both invalid inputs,
0and-1, so the API now has one consistent rule:OnlineEvaluator.max_concurrencymust be a positive integer, while intentional disabling remains represented by the existing sampling control such assample_rate=0.0.Evidence
Before this change, the underlying semaphore behavior is:
That means
max_concurrency=0is accepted at construction time but can never acquire a permit in the online evaluation dispatch path.After this change,
OnlineEvaluator(..., max_concurrency=0)andOnlineEvaluator(..., max_concurrency=-1)fail eagerly with:Possible call chain / impact
This PR only changes invalid
OnlineEvaluatorconfiguration. It does not change sampling, sink emission,on_max_concurrencybehavior for real concurrency pressure, or positive concurrency limits.Validation
uv run --project D:\ZXY\Github\pydantic-ai\pydantic_evals pytest D:\ZXY\Github\pydantic-ai\tests\evals\test_online.py -k "online_evaluator_invalid_max_concurrency or online_evaluator_custom_config or max_concurrency_respected"- passed, 4 testsgit diff --check- passedChecklist
Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
No breaking changes in accordance with the version policy.
PR title is fit for the release changelog.