Repository navigation
DiLoCo: min_workers pauses (quorum gate), not aborts - #237
Merged
Merged
Conversation
…ate) Replaces the #233 behavior (relay save_and_stop = abort) with a recoverable quorum gate, per the intended semantics: the group never takes an outer step with fewer than min_workers contributors — at startup before everyone has joined, OR after a drop — and pausing is recoverable (a replacement worker, or lowering min_workers), never a forced stop. Server: - _handle_worker_death no longer aborts below min_workers; it just logs. Removed _min_workers_aborted and the save_and_stop relay. - Heartbeat response carries below_min_workers (num_registered < min_workers). - Sync and fragment-sync barriers only apply a round when contributors >= min_workers (no partial outer step; async permits one extra to slip through). - Barrier waits break on _shutting_down, and graceful_shutdown wakes barrier- parked workers — a paused worker is always cleanly stoppable, never a hard unkillable block. - _below_min_workers() helper. Worker: - Reads below_min_workers off the heartbeat (_quorum_blocked) and, at its sync point, idles instead of syncing/training ahead while blocked — GPU parked, no garbage gradients. The idle polls _pending_stop() so save_and_stop/abort breaks it immediately (clean shutdown). _quorum_blocked starts False (optimistic): the server barrier gate is the authoritative no-partial-step enforcement, so a worker with no heartbeat loop (e.g. unit tests driving _sync directly) never idles forever. Limitation (noted in code/docs): assumes single rank per worker (the in-scope case); DDP-per-worker would need the pause broadcast to followers — a follow-up. Tests: replaced the abort tests with quorum-gate tests (drop => no abort, _below_min_workers, worker _pending_stop/_quorum_blocked). Full diloco suite green (594). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Critical (caught in review): the >= min_workers gate was on the two submit handlers but NOT on the _handle_worker_death barrier-release sites. A death that shrinks _round_expected_workers can flip _round_complete() True for the survivors, which then drove a partial outer step below quorum — the exact bug this PR exists to prevent. Gate both death-path releases (full-model + fragment) on contributors >= min_workers; below quorum, leave the survivors parked (the pause), released by a later submit once quorum returns or by _shutting_down. (+ regression test: min_workers=2, A submits, B dies -> no partial step, A stays parked, _sync_round unchanged.) Also: fixed the misleading _quorum_blocked test (it set the value it asserted and documented the opposite of the real optimistic default) — replaced with a heartbeat-flag-update test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Corrects the
min_workersbelow-quorum behavior. PR #233 made a drop abort the run (relaysave_and_stop); the intended behavior is a recoverable pause (block), never a forced stop. This reworks it into a quorum gate.Semantics (confirmed with the operator)
The group never takes an outer step with fewer than
min_workerscontributors — at startup (no one syncs until everyone has joined) or after a drop. Below quorum the run pauses (workers idle, GPU parked, no garbage gradients) and is recoverable (a replacement worker, or loweringmin_workers). Async permits one extra step to slip through as a worker drops; sync does not. Critically, a paused worker is never hard-blocked — it polls its stop flag, sodiloco shutdownis always clean.Server (
server.py)_handle_worker_deathno longer aborts belowmin_workers(removed_min_workers_aborted+ thesave_and_stoprelay).below_min_workers.>= min_workers._shutting_down;graceful_shutdownwakes barrier-parked workers._below_min_workers()helper.Worker (
worker.py)below_min_workersoff the heartbeat (_quorum_blocked) and idles at its sync point while blocked, polling_pending_stop()so a stop command breaks the pause immediately._quorum_blockedstartsFalse(optimistic) — the server barrier gate is the authoritative no-partial-step enforcement, so a worker driving_syncwith no heartbeat loop (unit tests) never idles forever. (That optimistic default is also the fix for a hang the pessimistic init caused in the test suite.)Limitation
Assumes single rank per worker (the in-scope case). DDP-per-worker would need the pause broadcast to followers — a follow-up (noted in code/docs).
Tests / docs
Replaced the obsolete abort tests with quorum-gate tests (drop ⇒ no abort,
_below_min_workers, worker_pending_stop/_quorum_blocked). Fulltests/unit/ml/diloco/green (594 passed).black/isortclean. Rewrote the architecture-docmin_workerssection.🤖 Generated with Claude Code