Skip to content

Commit 3759e05

Browse files
committed
fix(bench): make aider eval fn picklable for pebble workers
1 parent 01ee4fa commit 3759e05

2 files changed

Lines changed: 65 additions & 19 deletions

File tree

QA.md

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,41 @@
188188
matching feature list (`lang-core` or `lang-extra`) WITHOUT `dep:` prefix,
189189
add `#[cfg]`-gated insert in `tree_sitter_strategy.rs::LANGUAGE_CACHE`
190190

191+
## Windows CI Compatibility
192+
193+
- `fcntl` is Unix-only; `msvcrt.locking` is the Windows equivalent. For code
194+
that runs on CI (not on the bench server), guard Unix-only imports:
195+
196+
```python
197+
try:
198+
import fcntl as _fcntl
199+
except ImportError:
200+
_fcntl = None # type: ignore[assignment]
201+
```
202+
203+
- Bench jobs (common.py, diffctx_eval_fn.py) import `fcntl` only for the
204+
`_cache_lock` function — benchmark code never runs on Windows, but the
205+
test suite imports these modules. The `try/except` pattern keeps tests
206+
runnable on Windows while bench logic remains Linux-only.
207+
208+
## Bench Sweep Smoke QA
209+
210+
- **Always read GH Actions logs in full before diagnosing failures** — summary
211+
counts (n_ok/n_total) do not distinguish between bugs and expected infra failures.
212+
Two common false-positive patterns:
213+
- `oom_kill` (SIGKILL -9) on large repos like `astropy/astropy` — expected on GH
214+
runners with 7GB RAM; not a bug in the algorithm.
215+
- `clone_fail` / `clone_timeout` — transient network or GH rate-limit; retry or
216+
skip, not a code regression.
217+
- **Picklable eval functions** — eval_fn passed to pebble `ProcessPool.schedule()`
218+
must be picklable. Local closures (`def eval_fn(...)` inside a factory function)
219+
are NOT picklable. Use `functools.partial` + module-level function.
220+
Attaching `.shutdown` as a lambda is also not picklable — use a module-level
221+
`_noop_shutdown` function.
222+
- `make_aider_eval_fn` was fixed to use `functools.partial(_pool_eval_aider, ...)`.
223+
The `_AiderProcess` is now created lazily per-worker-process via a module-level
224+
`_AIDER_PROC` global (no inter-process sharing needed).
225+
191226
## actionlint context restrictions
192227

193228
- `${{ env.X }}` is BANNED inside `env:` blocks at any level (workflow, job,
@@ -242,8 +277,6 @@ this QA pass (touching benchmark code = regression risk on v1 results):
242277
`# noqa: C901` for ruff (orchestration with timeout/exception/checkpoint dispatch
243278
does not factor cleanly without behavior change)
244279
- `benchmarks/adapters/evaluator.py:49` (32→15)
245-
- `scripts/bake_bench_cache.py:72` (16→15)
246-
247280
Tactic from earlier passes (already in this file): extract `_collect_result`
248281
helpers and `_print_*_header`/`_print_*_dump` blocks. Pair with v2 evaluation
249282
re-run so any drift is caught.

benchmarks/baselines/aider_baseline.py

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,14 @@
55
hard-pins ~95 deps including litellm, numpy==1.26.4, fastapi — those would
66
break the main treemapper env if installed in-process).
77
8-
Spawn-once, reuse-many: a single helper process is kept alive across all
9-
instances in a run, so we pay aider's import cost (~1-2s) once per
10-
benchmark file, not 1500 times.
8+
Spawn-once, reuse-many: one helper process per worker process is kept alive
9+
across all instances assigned to that worker, so we pay aider's import
10+
cost (~1-2s) once per worker, not once per instance.
1111
"""
1212

1313
from __future__ import annotations
1414

15+
import functools
1516
import json
1617
import os
1718
import shutil
@@ -234,24 +235,36 @@ def _aider_eval(
234235
pass
235236

236237

238+
_AIDER_PROC: _AiderProcess | None = None
239+
240+
241+
def _noop_shutdown() -> None:
242+
pass
243+
244+
245+
def _pool_eval_aider(
246+
repos_dir_str: str,
247+
request_timeout: float,
248+
aider_mode: str,
249+
instance: BenchmarkInstance,
250+
params: RunParams,
251+
) -> EvalResult:
252+
global _AIDER_PROC
253+
if _AIDER_PROC is None:
254+
_AIDER_PROC = _AiderProcess()
255+
evaluator = UniversalEvaluator()
256+
worktree_dir = Path(repos_dir_str) / "worktrees" / f"w{os.getpid()}"
257+
worktree_dir.mkdir(parents=True, exist_ok=True)
258+
return _aider_eval(instance, params, evaluator, worktree_dir, _AIDER_PROC, request_timeout, aider_mode)
259+
260+
237261
def make_aider_eval_fn(
238262
repos_dir: Path,
239263
request_timeout: float = 300.0,
240264
aider_mode: str = "fair",
241265
):
242266
if aider_mode not in {"fair", "oracle"}:
243267
raise ValueError(f"aider_mode must be 'fair' or 'oracle', got {aider_mode!r}")
244-
245-
evaluator = UniversalEvaluator()
246-
worktrees_root = repos_dir / "worktrees"
247-
aider = _AiderProcess()
248-
249-
def eval_fn(instance: BenchmarkInstance, params: RunParams) -> EvalResult:
250-
import os
251-
252-
worktree_dir = worktrees_root / f"w{os.getpid()}"
253-
worktree_dir.mkdir(parents=True, exist_ok=True)
254-
return _aider_eval(instance, params, evaluator, worktree_dir, aider, request_timeout, aider_mode)
255-
256-
eval_fn.shutdown = aider.shutdown # type: ignore[attr-defined]
257-
return eval_fn
268+
fn = functools.partial(_pool_eval_aider, str(repos_dir), request_timeout, aider_mode)
269+
fn.shutdown = _noop_shutdown # type: ignore[attr-defined]
270+
return fn

0 commit comments

Comments
 (0)