async-eval: close 3 deferred Copilot comments (#3, #5, #12)

Three small fixes Copilot flagged in the original review that I
initially deferred as "minor". Each is a small, safe improvement
worth landing before merge so reviewers see zero open comments.

#3 — ``extra_args: null`` TypeError
  ``async_eval_config.py::from_dict``. ``list(sbatch_raw.get(
  "extra_args", []))`` would TypeError on ``list(None)`` when YAML
  explicitly sets ``sbatch: {extra_args: null}`` (common toggle
  pattern). Switched to ``or []`` so the dict-default and the
  explicit-null cases both yield ``[]``.
  Regression test: ``test_sbatch_extra_args_null_does_not_crash``.

#5 — fd leak in ``_respawn_server``
  ``reserved_callback.py``. The ``open(log_path, "ab")`` parent fd
  was never closed; Popen duplicates it into the child but the parent
  keeps its own copy. Leak = 1 fd per respawn over a long training
  run. Wrapped in a ``with`` block so the parent fd closes
  immediately after Popen forks the child (the child keeps its own).

#12 — misleading sum-vs-average assertion
  ``test_llm_generation_short_answer``. ``score == 1.0`` for two
  samples where only one is correct passes because the metric is a
  sum, not an average. Test was correct, just easy to misread.
  Added a clarifying comment + an additional assertion on the
  average (``score / count == 0.5``) to make the contract explicit.

Skipped #7 (reserved-mode ``_save_checkpoint`` evicting referenced
ckpts) — real edge case but only fires under reserved mode +
``on_overlap=queue`` with backed-up queue, neither of which is
exercised by the cookbook. Better as a separate issue if reserved
mode gets more usage.

438 passed / 12 skipped, full repo clean, ruff + pre-commit green.

Author: Rouzbehat78

src/leap_finetune/evaluation/async_eval_config.py

@@ -110,7 +110,10 @@ def from_dict(cls, raw: dict | None) -> "AsyncEvalConfig":
             partition=sbatch_raw.get("partition"),
             account=sbatch_raw.get("account"),
             time=str(sbatch_time) if sbatch_time else None,
-            extra_args=list(sbatch_raw.get("extra_args", [])),
+            # ``or []`` (not the dict-default) so an explicit ``null`` /
+            # ``~`` in YAML (common when toggling) doesn't TypeError into
+            # ``list(None)``.
+            extra_args=list(sbatch_raw.get("extra_args") or []),
         )
 
         reserved_raw = raw.get("reserved", {}) or {}

src/leap_finetune/evaluation/reserved_callback.py

@@ -227,23 +227,25 @@ def _respawn_server(self, ckpt_path: Path) -> None:
         env = _clean_subprocess_env()
         env["CUDA_VISIBLE_DEVICES"] = self.eval_gpu_ids
 
-        # Append server stdout+stderr across respawns so failures stay debuggable.
+        # Append server stdout+stderr across respawns so failures stay
+        # debuggable. Close the parent's fd as soon as Popen duplicates it
+        # into the child — otherwise we leak a fd per respawn over the
+        # lifetime of training.
         log_dir = self._eval_dir / "vllm_server"
         log_dir.mkdir(parents=True, exist_ok=True)
         log_path = log_dir / "server.log"
-        server_log = open(log_path, "ab")
-
         logger.info(
             "[async_eval/reserved] launching vLLM (log=%s): %s",
             log_path,
             " ".join(shlex.quote(c) for c in cmd),
         )
-        self._server_process = subprocess.Popen(
-            cmd,
-            env=env,
-            stdout=server_log,
-            stderr=subprocess.STDOUT,
-        )
+        with open(log_path, "ab") as server_log:
+            self._server_process = subprocess.Popen(
+                cmd,
+                env=env,
+                stdout=server_log,
+                stderr=subprocess.STDOUT,
+            )
 
     # === TrainerCallback hooks ===
 

tests/test_async_eval.py

@@ -117,6 +117,16 @@ def test_failure_retry_round_trip(self):
         assert cfg2.failure.max_submit_attempts == 7
         assert cfg2.failure.submit_retry_backoff == 0.5
 
+    def test_sbatch_extra_args_null_does_not_crash(self):
+        """``sbatch: {extra_args: null}`` is common when toggling YAML
+        values — must NOT TypeError into ``list(None)``."""
+        from leap_finetune.evaluation.async_eval_config import AsyncEvalConfig
+
+        cfg = AsyncEvalConfig.from_dict(
+            {"mode": "sidecar", "sbatch": {"extra_args": None}}
+        )
+        assert cfg.sbatch.extra_args == []
+
 
 # === Dispatch helper ===
 
@@ -236,8 +246,11 @@ def test_llm_generation_short_answer(self):
         result = bench.evaluate_with_backend(backend, samples)
         # First sample: "4" is contained in "the answer is 4" → 1.0
         # Second sample: "6" not in "no idea" → 0.0
+        # BenchmarkResult.metrics["score"] is the SUM (not the average);
+        # average = score / count = 1.0 / 2 = 0.5.
         assert result.count == 2
-        assert result.metrics["score"] == pytest.approx(1.0)
+        assert result.metrics["score"] == pytest.approx(1.0)  # sum of (1.0, 0.0)
+        assert result.metrics["score"] / result.count == pytest.approx(0.5)
 
     def test_llm_logprob_picks_argmax(self):
         from leap_finetune.evaluation.llm_benchmarks import LLMLogprobBenchmark