MoE Support by alay2shah · Pull Request #7 · Liquid4All/liquid-finetune

alay2shah · 2025-11-19T23:18:32Z

Tested with higher batch size and dataloaders
Works for SFT + DPO and LoRA
Integrates DeepSpeed for activation checkpointing + fused adam (measured ~15-20% improvement)

gemini-code-assist · 2025-11-19T23:18:36Z

Important

Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services.

EdoardoMosca · 2025-11-25T19:04:32Z

LGTM and is independent from previous modes

Full-fintunes don't work yet right?

EdoardoMosca

at least add error raise if user triggers moe finetuning with no lora. Ideally, add full finetune with moe as well

EdoardoMosca · 2025-12-19T18:30:08Z

@@ -0,0 +1,15 @@
+"""Utility functions for model detection and configuration."""


remove file-level docstring

EdoardoMosca

approved with one nit

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. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

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.

* eval: add InferenceBackend Protocol + HFBackend Introduce a narrow Protocol (generate + logprobs) so benchmarks can dispatch through the same code path whether they're talking to an in-process HF model, an in-process vLLM engine, or a remote vLLM server. Ships with HFBackend (in-process HF) used by the sync path and as a logprob fallback for the vLLM backends. Benchmark base class gains an additive evaluate_with_backend default (raises NotImplementedError) so subclasses can opt in incrementally. Zero behavior change for existing sync callers. * eval: benchmark backend dispatchers LLM and VLM generation + logprob benchmarks gain evaluate_with_backend implementations that build batched GenerateRequest / LogprobRequest lists, dispatch to an InferenceBackend, and score the responses with the existing per-sample scoring logic. Sync path is unchanged. Export the new backend symbols from leap_finetune.evaluation. * eval: async eval config + dispatch helper AsyncEvalConfig parses + validates the async_eval: YAML block (sync / sidecar / reserved) with sub-blocks for sbatch settings, reserved server settings, and failure handling. make_eval_callback dispatches to BenchmarkEvalCallback (sync), SidecarEvalCallback (sidecar), or ReservedEvalCallback (reserved). Sidecar and reserved imports are lazy so sync users don't pay the import cost. * eval: sidecar mode (sbatch per cycle, vLLM eval) SidecarEvalCallback (rank 0) stages a checkpoint, renders an sbatch script, and submits at every eval_steps. The sbatch job loads vLLM on whatever GPU SLURM assigns it, runs every configured benchmark, and back-fills the training run's wandb log at the originating step. Training never pauses on eval. A .in_flight marker enforces on_overlap policy (skip / queue); the sbatch clears the marker on EXIT so a crashed runner can't block the callback. After failure.max_consecutive failures the callback disables itself. When eval_on_start is true the step-0 sidecar runs synchronously (callback polls sacct until the job is terminal) so wandb's step counter stays aligned for the baseline metrics. * eval: reserved mode (long-running vLLM server) ReservedEvalCallback owns a daemon helper thread (rank 0) that hosts a persistent vLLM OpenAI server on the dedicated eval GPUs carved off the training pool. On each eval_steps the thread respawns the server against the latest checkpoint, runs every benchmark via VLLMServerBackend, and pushes results back to a queue. on_log drains the queue and back-fills wandb at the originating training step. on_train_end drains any in-flight cycles before teardown so results aren't dropped. Helper-thread exceptions never propagate to training. Single-node only; weight_reload=respawn only (in_place rejected with a clear error). Driver-side GPU carving lands in the next commit. * eval: driver-side reserved-mode GPU carving For mode=reserved, the driver carves vllm_gpus off the training pool at job start, sets CUDA_VISIBLE_DEVICES for the trainer accordingly, and hands the worker the eval server URL + carved GPU ids through train_loop_config. The worker (rank 0) launches its own vllm-serve subprocess inside the helper thread so it owns the lifetime and can respawn on weight reload. Runs AFTER any GRPO server-mode carve so the two modes can coexist. Multi-node is rejected with a clear error. * eval: parse async_eval block + wire training loops config_parser validates the async_eval YAML block on the driver (misconfig errors surface before a Ray worker is spawned). The raw dict is forwarded into train_loop_config. Each of the 5 training loops (sft, dpo, grpo, vlm_sft, vlm_grpo) replaces its direct BenchmarkEvalCallback registration with the make_eval_callback dispatch helper. Same call shape across loops; the dispatcher picks sync / sidecar / reserved based on the YAML. * eval: tests + example YAML for async eval Unit tests for AsyncEvalConfig parsing, make_eval_callback dispatch, sidecar marker lifecycle, and FakeBackend round-trip through the benchmark dispatchers. Toy fixtures (sidecar.sh / reserved.sh + matching YAMLs) exercise each mode end-to-end on a single GPU against a tiny QA benchmark under SLURM. job_configs/sft_with_async_eval_example.yaml is the copy-paste starting point users land on from the README. * docs: async eval README section Top-level overview of the three modes (sync / sidecar / reserved): when to pick each, the trade-offs (training pause vs reserved GPUs vs queue latency), and the YAML schema. Points users at the example config + the per-mode behavior contracts. * style: ruff + prettier auto-format No behavior change. Wraps long argparse / log-format lines, normalizes frozenset literal layout, drops an unused import, and prettier-aligns the async eval table + YAML snippet in the README. * docs: note that reserved mode is single-node only Adds an Engine + Multi-node column to the mode table and a paragraph explaining the constraint comes from the driver's CUDA_VISIBLE_DEVICES only affecting the head node. Multi-node training should use sidecar, which already scales transparently to any node count. * async-eval: log without step= so wandb keeps benchmark points Passing step=trigger_step to wandb.log silently drops the entry when the run's internal _step has advanced past trigger_step. The sidecar resumes the run minutes after the trigger fired, and GRPO commits many times per training step, so _step often blows past trigger_step (we saw step=2000 dropped at _step=42406 with "this data will be ignored"). SFT runs avoided this only because their _step stayed below trigger_step. Drop the explicit step= and tag each point with two plain data fields: train/global_step (what existing dashboards already use as the x-axis) and benchmark/step (a clean alias). The write is now always forward — appends at the current _step — and panels still render benchmark points at the originating training step. reserved_callback also drops commit=False — the benchmark point was relying on a follow-up training log to flush it, which silently lost the point if the eval was on the last step. * test: fix stale async-eval test assertions (3 tests) * async-eval: auto-pin benchmark/* to benchmark/step axis * async-eval: tighten verbose comments around wandb logging * async-eval: sidecar sbatch retry + new VLM grounding metric - sidecar_callback: wrap the sbatch call in an exponential-backoff retry loop. Transient slurm errors (controller busy, brief network blip) no longer lose a benchmark point. Knobs in FailureConfig: max_submit_attempts (default 3), submit_retry_backoff (default 2.0s, doubles each retry). FileNotFoundError ("sbatch" missing) still fails fast — that's a permanent config issue. - async_runner_main: pin every benchmark key to benchmark/step BEFORE first log so wandb auto-renders the panels on the correct axis. Trailing benchmark/* glob as safety net (wandb requires globs to be suffix-only, so benchmark/*/* is rejected). - reserved_callback: trim verbose comment block; keep the axis pin. - metrics: add Hungarian-matched grounding_iou_f1 metric for multi-bbox prompts. Reduces to single-bbox IoU naturally on 1-vs-1 cases. - vlm_config: thread the new metric through to AsyncEvalConfig. * async-eval: fix marker leak + retry/metric tests + tidy Bug (caught by Codex review): SidecarEvalCallback wrote the .in_flight marker BEFORE sbatch succeeded and only cleaned it on full local failure or via the remote shell trap. If Python died between marker write and submit, or the slurm job died before its trap (slurmstepd OOM, NODE_FAIL, scancel --signal=KILL), the orphan marker permanently blocked future evals under on_overlap=skip. - sidecar_callback._submit: write marker only AFTER a successful sbatch return, with payload "<jobid>:<step>" so the recovery path can ask sacct about the recorded job. - sidecar_callback._clear_marker_if_stale (new): called at the top of _fire. If sacct says the recorded job hit a terminal state, drop the marker. If sacct is missing or unparseable, fall back to a 6h mtime cutoff. - async_runner_main / reserved_callback: bump the swallowed define_metric failure from logger.debug to logger.warning so silent axis-pin failure is visible at the default INFO level. - async_eval_config: document submit_retry_backoff=0 as "immediate burst, only safe on quiet controllers". Tests: - tests/test_grounding_metrics.py (new, 22 cases): metric coverage was zero prior. Covers _parse_bboxes strictness (prose-preamble rejection, bare-list rejection, range / geometry checks), _hungarian_match_iou (optimal-vs-greedy, mismatched lengths, scipy-missing fallback), score_grounding_iou_f1 boundary cases (empty pairs, perfect / no overlap, multi-permuted, extra-pred drags precision, missing-pred drags recall), dispatch-table wiring, legacy single-bbox metric. - tests/test_async_eval.py: 11 new cases — FailureConfig round-trip with retry knobs, sidecar sbatch retry loop (success-first / retry-then- succeed / exhaust-no-marker-left / FileNotFoundError-fail-fast / zero-backoff / one-retry-per-step semantics), stale-marker recovery (sacct terminal / sacct running / mtime fallback), wandb define_metric ordering and per-key enumeration. All 57 tests pass; ruff + pre-commit clean. * async-eval: stale-marker recovery must never delete a live job Codex flagged a second-order bug in the recovery path I just added: if sacct returned a non-terminal state (RUNNING / PENDING / COMPLETING), the loop didn't return, so the code fell through to the 6h mtime cutoff. For long-queued evals or big benchmark suites, the marker can be older than 6h while the job is still alive — the mtime fallback would then delete a live sidecar's marker and the next _fire would submit a duplicate eval for the same checkpoint. Fix: when sacct returns rc=0 with non-empty stdout, treat it as authoritative. Clear on terminal state, otherwise return without touching the marker. The mtime fallback only fires when sacct was unreachable (FileNotFoundError / TimeoutExpired / no output). Regression test: test_preserves_live_job_even_with_old_marker — backdates the marker by 7h and stubs sacct→RUNNING; asserts marker survives. The prior test_preserves_when_sacct_reports_running used a fresh marker and silently passed despite the bug. * async-eval: active-state allowlist for stale-marker recovery Codex flagged that the prior _SACCT_TERMINAL_STATES set was incomplete (missing BOOT_FAIL, DEADLINE, REVOKED, SPECIAL_EXIT and any future slurm states). If a job hit one of those, _clear_marker_if_stale's "sacct-authoritative" branch found no terminal-set match and returned early treating the job as alive — stranding the marker forever and blocking every subsequent eval under on_overlap=skip. - Switch to an allowlist of ACTIVE states (PENDING, RUNNING, SUSPENDED, COMPLETING, CONFIGURING, RESIZING, SIGNALING, STAGE_OUT, REQUEUED+ variants). Anything outside — including recognized terminal states and unknown future ones — is treated as terminal and the marker is cleared. Default-to-cleared avoids the strand-forever failure mode. - Also strip slurm's trailing "+" suffix and uppercase before matching so e.g. "RUNNING+" still classifies correctly. - Expand _SACCT_TERMINAL_STATES to the canonical set for _wait_for_job's substring exit check. Regression tests: parametrized BOOT_FAIL/DEADLINE/REVOKED/SPECIAL_EXIT/ MYSTERY_STATE all clear marker; mixed RUNNING+COMPLETED rows keep marker; trailing "+" doesn't defeat active matching. 65/65 tests pass; full repo suite 449 passed 11 skipped, no regressions. * metrics: restore permissive _parse_bbox for legacy grounding_iou Codex caught: routing _parse_bbox through the new strict _parse_bboxes silently broke prediction formats that score_grounding_iou used to accept. The strict parser is reward-aligned (must mirror the GRPO reward's _validate_bbox), which is correct for grounding_iou_f1 — but grounding_iou scores published baselines (refcoco trio etc.) whose output predates the cookbook recipe. Restored formats: - prose-embedded JSON via regex extraction (`"Box: [0,0,1,1]"`) - bare `[x,y,x,y]` lists and `[[x,y,x,y]]` list-of-lists - single top-level dict `{"bbox":[...]}` - ast.literal_eval fallback for Python-literal-but-not-JSON - 0-1000 MGrounding-native coord space (autoscaled to 0-1) grounding_iou_f1 still uses the strict _parse_bboxes directly — the two metrics now have explicit, divergent strictness contracts: * grounding_iou (legacy): permissive, baseline-friendly * grounding_iou_f1 (new): strict, reward-aligned Regression tests in TestGroundingIouLegacy pin all six accepted formats so future refactors can't quietly re-tighten the legacy path. 456/456 tests pass (up from 449), ruff + pre-commit clean. * metrics: harden permissive _parse_bbox against malformed-input crashes Codex flagged a silent-inflation path on the legacy ``grounding_iou`` metric: the restored permissive parser would raise on malformed predictions (e.g. ``{"bbox": 42}`` → ``TypeError: object of type 'int' has no len()``). ``Benchmark.evaluate`` excludes per-sample failures from the count, so a parser exception silently DROPS the sample instead of scoring it as 0 — inflating the running mean. Hardening: - Wrap the parser in a catch-all returning None on any unexpected exception (defense in depth for shapes I haven't enumerated). - isinstance(bbox, (list, tuple)) check before len() — covers the documented crash on int/float/dict bbox values. - Reject NaN/Inf coords with math.isfinite — would otherwise poison _compute_iou's intersection/union arithmetic downstream. - Skip non-dict items inside the list-of-dicts branch. Regression tests in TestGroundingIouMalformedDoesNotInflate pin each documented crash path (int bbox, dict bbox, string bbox, NaN, Inf) plus a catch-all sweep of 12 pathological inputs that must all return None. 462/462 tests pass (up from 456), full repo suite clean, ruff + pre-commit green. * metrics: reject boolean bbox coords before float() coerces them Codex flagged: a model emitting JSON booleans (e.g. ``[false, false, true, true]``) coerces through ``[float(x) for x in bbox]`` to a clean-looking ``[0.0, 0.0, 1.0, 1.0]`` and scores IoU=1.0 against a full-image GT — a free perfect score from semantic gibberish, because ``bool`` is a subclass of ``int`` and ``isinstance(True, int)`` is True. Added a bool-rejection pass right after the list/tuple shape check. Applies before float conversion so True/False can't sneak through as 1.0/0.0. Regression test ``test_boolean_coords_rejected_not_coerced`` pins both the bare-list and list-of-dicts paths; both return None from the parser and 0.0 from ``score_grounding_iou`` against [0,0,1,1] GT. 463/463 tests pass. * tests: trim async-eval + grounding-metric suites to contract-pinning core Aggressive cut of redundant test coverage I had piled on under Codex review pressure. Kept only: - User-visible contract tests (1 per metric/feature, not exhaustive enumeration of symmetric cases) - Codex-caught regression pins (bool coercion, marker-leak, live-job preservation under stale-marker recovery, unenumerated terminal states) - Environment contracts (scipy-or-greedy Hungarian fallback) Dropped: - TestParseBboxes and TestHungarianMatch internal-helper enumerations — covered transitively by score_grounding_iou_f1 e2e tests - Symmetric boundary cases (perfect-match + no-overlap, extra-pred + missing-pred drag, empty/nonempty either direction) - Multiple FailureConfig round-trip variants → consolidated to one - Multiple sbatch-retry edge cases (zero-backoff, exact backoff values) → kept only happy-path, exhaustion, and FileNotFoundError - Stale-marker recovery: dropped sacct-missing-mtime + mixed-rows + suffix-stripping internal-detail tests, kept clear/preserve/unknown - Wandb axis: dropped per-key enumeration, kept ordering pin Suite size: 463 → 425 (38 tests removed). Full repo runs in 120s, zero regressions in existing capabilities. * tests: restore 6 contract-pinning tests dropped in the trim Codex flagged that the previous trim removed non-redundant coverage. Restored only the tests that pin a UNIQUE behavior not exercised transitively by other tests: In test_grounding_metrics.py (new TestStrictParser + TestHungarianMatching): - test_rejects_out_of_range_coords: strict parser range check. F1 e2e tests always use valid 0-1 coords, so a relaxed range check would go undetected. - test_rejects_zero_or_inverted_area: same gap — geometry check has no e2e probe. - test_picks_optimal_assignment_not_greedy: a regression to greedy matching would still pass test_multi_permuted_still_matches (both algorithms find the all-or-nothing perfect match). Pin the optimal-vs-greedy contrast directly on a 2x2 adversarial matrix. In test_async_eval.py: - test_clears_when_sacct_missing_and_marker_old: third branch of _clear_marker_if_stale. The sacct-available paths above don't exercise the mtime fallback. - test_keeps_marker_when_any_row_active_in_mixed_output: multi-row sacct output (parent + .batch step). Single-row tests wouldn't catch an "only first row" regression. - test_define_metric_enumerates_every_benchmark_key: per-key vs glob-only. The ordering test would still pass if someone reverted to a single glob — but per-key registration would silently break because define_metric on an already-logged key is a no-op. Suite: 425 → 431 (+6), full repo runs in 122s, zero regressions. * tests: rewrite Hungarian-vs-greedy test to actually catch a greedy regression Codex flagged that the previous version of this test would pass under a greedy regression. Root cause: I used a bbox config whose IoU matrix is "near-monotone" — greedy and Hungarian both give 2.0, so swapping the algorithm wouldn't change the assertion. The 2D IoU geometry can't reproduce the textbook greedy-suboptimal matrix ([[1.0, 0.9], [0.9, 0.1]]) from real bboxes because triangle-inequality-ish constraints couple pred[0]'s closeness to multiple gts with pred[1]'s closeness. Fix: monkeypatch _compute_iou to inject the adversarial matrix directly, bypassing geometric constraints. Greedy: picks (0,0)=1.0 → forced to (1,1)=0.1 → sum 1.1 Optimal: cross (0,1)+(1,0) = 0.9+0.9 → sum 1.8 Skipped when scipy is missing — there the greedy fallback IS the implementation and the optimal-vs-greedy contract doesn't apply. Confirmed locally: the same mocked matrix returns 1.1 from the greedy fallback, so a real regression would flip the assertion. Suite: 430 passed / 12 skipped (the +1 skip is this test in the scipy-less dev env). * async-eval: address Copilot review — paths + 3 contract bugs Copilot caught 11 real issues; the 4 hardcoded /home/rouzbeh paths in test fixtures + 3 contract-breaking bugs are addressed here. The 4 minor ones (fd leak, null extra_args, ckpt-deletion race, logprob image error handling) deferred to a follow-up. Hardcoded paths (4 fixtures): - tests/fixtures/toy_async_eval_{sidecar,reserved}.yaml: relative ``tests/fixtures/tiny_qa_bench.jsonl`` - tests/fixtures/toy_async_eval_{sidecar,reserved}.sh: ``cd $SLURM_SUBMIT_DIR`` instead of hardcoded checkout path; module load via ``LEAP_CUDA_MODULE`` env var; CUDA_HOME default to /usr/local/cuda Contract bugs: 1. sidecar ``on_overlap=queue`` marker race (sidecar_callback.py). Single shared ``.in_flight`` marker meant the first sidecar's EXIT trap wiped concurrent siblings. Fix: per-step markers (``.in_flight.step_<N>``); script trap removes only its own; ``_fire`` checks ``glob(_MARKER_GLOB)``; ``_sweep_stale_markers`` iterates all and sacct-clears the dead. Honest log message: "N eval(s) in flight; submitting step <X> anyway (on_overlap=queue)". 2. reserved-mode helper-thread failures not counted (reserved_callback.py). ``_run_loop`` catches exceptions and emits an empty ``metrics`` dict; ``_consecutive_failures`` was never incremented despite the docstring claiming auto-disable. Fix: new ``_account_result`` called from all three drain sites; empty metrics increment, non-empty reset. 3. vlm_benchmarks sample misalignment (vlm_benchmarks.py). ``VLMGenerationBenchmark`` filters unreadable-image samples while building requests/ground_truths but scored against the original samples list, shifting indices. Fix: parallel ``kept_samples`` list. Same image-error handling added to ``VLMLogprobBenchmark``. Also: wandb/ added to .gitignore so local run logs can't be swept into commits via ``git add -A``. Tests: 3 new contract pins (TestSidecarConcurrentMarkers, TestReservedFailureAccounting). Updated existing tests to per-step marker name. 433 passed / 12 skipped, full repo clean. * async-eval: fix reserved failure accounting — empty metrics ≠ failure Codex flagged: my previous fix treated ANY empty ``metrics`` dict as a failure, but ``_run_one_cycle`` legitimately returns ``{}`` in healthy cases too: * all benchmarks had no samples loaded * all benchmarks raised NotImplementedError (backend doesn't support that benchmark type) and got skipped * all benchmarks had count=0 Treating those as failures auto-disabled working setups. Fix: add an ``ok: bool = True`` field to ``_EvalResult``. The ``_run_loop`` exception branch sets ``ok=False``; ``_account_result`` counts only ``ok=False`` toward ``_consecutive_failures``. Empty metrics with ``ok=True`` reset the counter (healthy cycle). Regression test ``test_empty_metrics_with_ok_true_does_NOT_count`` pins it: 10 healthy no-metric cycles in a row must not flip ``_disabled`` even with ``max_consecutive=2``. 434 passed / 12 skipped, full repo clean. * async-eval: classify all-benchmarks-raise as a real cycle failure Codex flagged: ``_run_one_cycle`` silently catches per-benchmark Exceptions and continues, so if EVERY benchmark raised a real (non- NotImplementedError) error, the cycle returned ``{}`` with ok=True. With the previous fix that treats ok=True as healthy, that's a silent total-failure being hidden — auto-disable can never fire. Fix: ``_run_one_cycle`` now returns ``(results, ok)``. Track ``real_errors`` inside the per-benchmark loop; ``ok = bool(results) or real_errors == 0`` correctly classifies: - any metrics produced → ok (full or partial success) - no metrics, no real errors → ok (healthy no-op: no samples or NotImplementedError-skipped) - no metrics, ≥1 real errors → NOT ok (every attempted benchmark raised; auto-disable should count this) Regression tests in new TestRunOneCycleClassification pin all three paths via mocked benchmarks and stubbed _respawn_server + _wait_for_health: all-raise → ok=False, all-NotImplementedError → ok=True, partial-failure (1 raise + 1 success) → ok=True. * async-eval: stop on_evaluate from wiping the cycle-failure counter Codex flagged the auto-disable chain is STILL broken end-to-end: even with the ok=True/False classification correct, ``on_evaluate``'s successful-submit branch was doing ``self._consecutive_failures = 0``. But the same counter is shared with helper-thread cycle failures drained in ``_account_result``. Every successful submit wiped the running cycle-failure count, so repeated broken cycles could never accumulate to ``max_consecutive``: on_evaluate(1) → submit OK → counter = 0 helper raises → drain → ok=False → counter = 1 on_evaluate(2) → submit OK → counter = 0 ← bug wipes it helper raises → drain → ok=False → counter = 1 (forever stuck, never disables) Fix: drop the ``self._consecutive_failures = 0`` on successful submission. The counter now resets ONLY on a successful cycle drain (``_account_result`` with ok=True). Submission failures still increment via the on_evaluate except branch — so a series of submit failures still disables correctly, and a series of cycle failures finally disables correctly too. Regression test ``test_disable_after_repeated_broken_cycles_end_to_end``: drives on_evaluate + _account_result alternately with mocked submit prerequisites; asserts the counter accumulates across cycles and hits ``_disabled = True`` at ``max_consecutive``. Would have caught the reset-clobber. 438 passed / 12 skipped, full repo clean. * 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. * async-eval: sidecar auto-disable now fires on dead sidecars The toy end-to-end test surfaced exactly the gap Codex flagged: sidecars submitted successfully but died in <2s (rocm dep resolver error inside the sidecar's uv run), and the callback didn't disable because the sidecar-mode failure counter only tracked SUBMISSION errors. Dead sidecars detected by ``_sweep_stale_markers`` were cleared but never counted. This is the parallel of the reserved-mode bug already fixed: each ``_fire`` was also doing ``self._consecutive_failures = 0`` on successful submit, which would wipe any sweep-detected failures. Fixes: 1. ``_clear_marker_if_stale`` returns ``bool`` (True iff cleared). ``_sweep_stale_markers`` aggregates the clears and increments ``_consecutive_failures`` accordingly — each cleared orphan is a sidecar whose bash trap didn't fire (slurmstepd OOM, NODE_FAIL, scancel --signal=KILL). 2. ``_fire`` no longer resets the counter on successful submit. The counter only ratchets upward; submission failures AND sweep-cleared orphans both contribute to the auto-disable threshold. Sidecar mode has no positive feedback signal (bash trap removing a marker isn't observable to us), so any reset would risk wiping unprocessed failures. 3. ``sbatch_template.py``: sidecar sub-jobs now ``uv run --no-sync`` to reuse the parent's resolved venv. Without this, an upstream wheel-index churn (e.g. vllm rocm wheels yanked) crashes every sidecar before it starts. Matches the cluster convention already used in the cookbook launchers per CLAUDE.md. 4. Toy fixture launchers (``toy_async_eval_*.sh``) also switched to ``uv run --no-sync``. Regression tests: - ``test_dead_sidecars_disable_callback`` — drops 3 orphan markers, stubs sacct=FAILED, asserts counter=3 and ``_disabled=True``. - Existing ``test_failure_disables_after_max_consecutive`` still pins the submit-failure path (both modes accumulate). 440 passed / 12 skipped, full repo clean. * Port async eval to unified config --------- Co-authored-by: alay2shah <alay0shah@gmail.com>

alay2shah added 7 commits November 13, 2025 18:22

transformers bump

070d65c

config updates

ccfd335

config readme

6710654

disable use cache for act checkpointing

5820382

Moe DPO + DS configs

b2260a1

pid added to triton cache

900c85c

MoE SFT + DS

1ccb376

alay2shah requested a review from EdoardoMosca November 20, 2025 01:44

EdoardoMosca requested changes Dec 1, 2025

View reviewed changes

alay2shah added 10 commits December 9, 2025 18:11

fsdp dpo

beaeb28

fsdp sft

a7ab997

add grad accum steps to user config

9866b51

dtype change

c19c39a

util for moe detection

2bf2339

ray tmp

996c37e

fsdp moe config

065e7c7

adding moe to sft run

786afdd

adding moe to dpo run

8148e06

moe configs

01951ae

EdoardoMosca reviewed Dec 19, 2025

View reviewed changes

EdoardoMosca approved these changes Dec 19, 2025

View reviewed changes

rm docstring

8b91818

alay2shah merged commit 88b4f94 into main Dec 29, 2025
1 check passed

alay2shah deleted the moe-support branch December 29, 2025 18:56

Navigation Menu

Uh oh!

MoE Support - #7

MoE Support#7
alay2shah merged 18 commits into
mainfrom
moe-support

alay2shah commented Nov 19, 2025

Uh oh!

gemini-code-assist Bot commented Nov 19, 2025

Uh oh!

EdoardoMosca commented Nov 25, 2025

Uh oh!

EdoardoMosca left a comment

Uh oh!

EdoardoMosca Dec 19, 2025

Uh oh!

EdoardoMosca left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,15 @@
		"""Utility functions for model detection and configuration."""

Uh oh!

Conversation

alay2shah commented Nov 19, 2025

Uh oh!

gemini-code-assist Bot commented Nov 19, 2025

Uh oh!

EdoardoMosca commented Nov 25, 2025

Uh oh!

EdoardoMosca left a comment

Choose a reason for hiding this comment

Uh oh!

EdoardoMosca Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

EdoardoMosca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants