perf(experiments): parallelize Phase 1 subject calls in Evaluator#4853
Merged
Conversation
github-actions
Bot
added
rust
This was referenced Jun 5, 2026
Closed
github-actions
Bot
added
the
documentation
bug-ops
force-pushed
the
4794-evaluator-parallel-phase1
branch
from
6df25c2 to
783a9e0
Compare
Subject model calls in Evaluator::evaluate were sequential. Mirror the existing Phase 2 pattern: use FuturesUnordered + Arc<Semaphore> bounded by the parallel_evals field (default 3) for both phases. After all subject futures complete, sort by case index to restore deterministic ordering before Phase 2 begins. Error semantics unchanged: any subject failure (Llm or Timeout) is fatal and propagates immediately. Closes #4794
bug-ops
force-pushed
the
4794-evaluator-parallel-phase1
branch
from
783a9e0 to
a1b9dfe
Compare
Merged
7 tasks
bug-ops
added a commit
that referenced
this pull request
Jun 5, 2026
…prove evaluator tests Closes #4870, #4854, #4855 Fix total_tokens overcounting (#4870): Evaluator::evaluate reserved a +1 slot per admitted judge call to prevent TOCTOU budget overshoot. The reservation was never cancelled, causing total_tokens to be overstated by the number of successful judge calls. Fix applies the correction at report-build time: raw counter is adjusted by saturating_sub(cases_scored) after all futures join, keeping the budget guard conservative during execution. Fix concurrency limit test (#4854): parallel_eval_respects_concurrency_limit declared a peak counter but never incremented it, so the assertion proved only absence of deadlock. MockProvider now tracks in-flight calls with atomics and records the peak via fetch_max. Test asserts both peak <= parallel_evals and peak >= parallel_evals (lower bound prevents silent serialization regressions). Add ordering test (#4855): new test subject_responses_ordered_after_parallel_phase1 uses per-call delays so subject futures complete in reverse order, then asserts that per_case[i].case_index == i for all i. Verifies the sort_unstable_by_key invariant introduced in PR #4853. Also make MockProvider::chat yield_now() conditional on delay > 0 to avoid scheduler preemption in zero-delay sequential callers.
bug-ops
added a commit
that referenced
this pull request
Jun 5, 2026
…prove evaluator tests Closes #4870, #4854, #4855 Fix total_tokens overcounting (#4870): Evaluator::evaluate reserved a +1 slot per admitted judge call to prevent TOCTOU budget overshoot. The reservation was never cancelled, causing total_tokens to be overstated by the number of successful judge calls. Fix applies the correction at report-build time: raw counter is adjusted by saturating_sub(cases_scored) after all futures join, keeping the budget guard conservative during execution. Fix concurrency limit test (#4854): parallel_eval_respects_concurrency_limit declared a peak counter but never incremented it, so the assertion proved only absence of deadlock. MockProvider now tracks in-flight calls with atomics and records the peak via fetch_max. Test asserts both peak <= parallel_evals and peak >= parallel_evals (lower bound prevents silent serialization regressions). Add ordering test (#4855): new test subject_responses_ordered_after_parallel_phase1 uses per-call delays so subject futures complete in reverse order, then asserts that per_case[i].case_index == i for all i. Verifies the sort_unstable_by_key invariant introduced in PR #4853. Also make MockProvider::chat yield_now() conditional on delay > 0 to avoid scheduler preemption in zero-delay sequential callers.
bug-ops
added a commit
that referenced
this pull request
Jun 5, 2026
…prove evaluator tests (#4888) Closes #4870, #4854, #4855 Fix total_tokens overcounting (#4870): Evaluator::evaluate reserved a +1 slot per admitted judge call to prevent TOCTOU budget overshoot. The reservation was never cancelled, causing total_tokens to be overstated by the number of successful judge calls. Fix applies the correction at report-build time: raw counter is adjusted by saturating_sub(cases_scored) after all futures join, keeping the budget guard conservative during execution. Fix concurrency limit test (#4854): parallel_eval_respects_concurrency_limit declared a peak counter but never incremented it, so the assertion proved only absence of deadlock. MockProvider now tracks in-flight calls with atomics and records the peak via fetch_max. Test asserts both peak <= parallel_evals and peak >= parallel_evals (lower bound prevents silent serialization regressions). Add ordering test (#4855): new test subject_responses_ordered_after_parallel_phase1 uses per-call delays so subject futures complete in reverse order, then asserts that per_case[i].case_index == i for all i. Verifies the sort_unstable_by_key invariant introduced in PR #4853. Also make MockProvider::chat yield_now() conditional on delay > 0 to avoid scheduler preemption in zero-delay sequential callers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
for-loop inEvaluator::evaluatePhase 1 (subject model calls) withFuturesUnordered+Arc<Semaphore>, mirroring the existing Phase 2 (judge) parallelization pattern exactly.parallel_evalsfield (default: 3) — no new config fields.sort_unstable_by_key(|(i, _)| *i)before Phase 2.Performance (before/after)
The hill-climbing loop in
ExperimentEngine::run_loopcan now evaluate roughly 3× more variation candidates within the samemax_wall_time_secsbudget.Implementation notes
AnyProvider::clone()resetsUsageTrackerto default — each parallel future has genuinely task-local state, no shared mutable state through the clone._permitis dropped per-future so all semaphore permits are released correctly.Follow-up issues (non-blocking, filed separately)
parallel_eval_respects_concurrency_limittest does not assert the concurrency ceiling for Phase 1.Closes #4794