Skip to content

Explicit pool claim/reset: one trainer call owns one pool epoch - #5

Merged
jvmncs merged 2 commits into
mainfrom
devin/1782792683-pool-claim-reset
Jun 30, 2026
Merged

Explicit pool claim/reset: one trainer call owns one pool epoch#5
jvmncs merged 2 commits into
mainfrom
devin/1782792683-pool-claim-reset

Conversation

@jvmncs

@jvmncs jvmncs commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Makes pool ownership explicit and restart-safe: one trainer launch ↔ one run ↔ one pool epoch. Today the pool's reset-to-base is inferred from a run_id mismatch on the first publish, and two cookbooks write latest two different ways — so a crash-restart that reused a run_id (derived from a dir basename) could leave the pointer pinned to a dead incarnation's high-water version, and "who owns pool state" was genuinely ambiguous.

This PR introduces a single pointer-control vocabulary all writers share, an explicit claim (write the empty pointer at launch), and a minimal no-Modal harness that pins the invariants down with tests.

Shared decision in protocol.py

One rule both writer paths go through, so they can't bake in divergent semantics:

BASE_VERSION = 0  # the "empty" pointer a launch claims (forks at base)

def decide_pointer_move(current_run_id, current_version, *, run_id, version) -> PointerMove:
    if run_id != current_run_id:
        return PointerMove(run_id, version, reset=True)   # new run forks at base (incl. claim)
    if version <= current_version:
        raise PointerRewind(...)                          # monotonic within a run
    return PointerMove(run_id, version, reset=False)

Board: guarded writers (bulletin.py)

board.advance(run_id, version)  # decide_pointer_move + write_latest; raises PointerRewind on rewind
board.claim(run_id)             # advance(run_id, BASE_VERSION): write <run_id>/weight_v000000

claim resets every replica to base before any delta. The reconcile path is unchanged — the pool still resets on a run_id switch via WeightSyncManager._switch_run (re-materialize base under the commit gate).

Consolidation (de-divergence)

  • frontdoor advance_latest_decision now delegates to decide_pointer_move (only parses the wire identity + shapes the error dict). modal_serve.advance_to writes through board.write_latest instead of a third raw latest write.
  • bulletin_hooks commit_and_wake publishes via board.advance (monotonic; new-run first publish forks at base) instead of an unconditional write_latest; adds claim_pool (rank-0 launch hook: claim → commit volume → wake). slime/miles hooks.py get thin claim_pool wrappers and their modal_train launches call it before the trainer starts publishing.
  • _run_id is now required (passed via custom_config) — the dir-basename fallback (the reuse hazard) is gone. Both disagg trainers already mint a fresh uuid4 per launch.

A restart is now just a new epoch: a fresh run_id claims and resets the pool; reusing a run_id raises PointerRewind instead of silently serving stale weights.

Minimal config: cookbook/local_disagg/

No Modal, no slime/miles, no GPUs — a filesystem board, an in-memory pool (MemoryEngine + real WeightSyncManager), and a trainer-as-writer using the same claim/advance/reconcile primitives. harness_test.py asserts the invariants:

  • claim resets the pool to base (v0) under the new run's id
  • within a run the pool converges to each published version in order
  • a new run forks at base even from a higher prior version (engine reset observed)
  • re-claiming a reused run_id is rejected as a rewind; a fresh run_id claims clean
  • non-monotonic publish within a run is rejected
  • a late-joining (cold/scaled-up) replica reconciles to the current run

Testing

uv run python -m pytest -q → 98 passed (was 82; +16 new tests across protocol_test, bulletin_test, frontdoor_test, and the new harness_test). Modal-only cookbook files (modal_train.py, hooks, modal_serve.py) py_compile-check clean. No lint config or CI workflows exist in the repo; pytest is the gate.

Link to Devin session: https://modal.devinenterprise.com/sessions/b0d6110c71924c24943bcb5cea1eb5f4
Requested by: @jvmncs

@jvmncs jvmncs self-assigned this Jun 30, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

devin-ai-integration Bot and others added 2 commits June 30, 2026 06:59
Co-Authored-By: jason.mancuso@modal.com <jvmncs@gmail.com>
After rebasing onto main (which includes #4, composing a multi-version
catch-up into a single engine reload), two reconcile tests still asserted
the pre-#4 per-version apply accounting:

- local_disagg/harness_test.py (added by this PR): a cold latecomer
  catching up v1..v2 now records a single composed apply [2], not [1, 2].
- standalone_rollouts/provider_test.py (pre-existing; #4 changed the
  behavior but did not update this cookbook test): same one-apply tail.

The served version / run_id invariants are unchanged; only the per-apply
accounting moved. No production code change.

Co-Authored-By: jason.mancuso@modal.com <jvmncs@gmail.com>
@devin-ai-integration
devin-ai-integration Bot force-pushed the devin/1782792683-pool-claim-reset branch from b5ddb3f to de61cce Compare June 30, 2026 07:03
@jvmncs
jvmncs merged commit 21674a5 into main Jun 30, 2026
jvmncs added a commit that referenced this pull request Jul 4, 2026
* Remove dead and speculative code from the core library

- generate_rollout compatibility wrapper and the unreachable per-request
  version-pinning branch (the live hook contract never supplies rollout_id);
  the request hook now does only what runs: retry budget + session affinity
- test-only helpers: transition_artifact_paths, parse_extra_key_version,
  validate_policy (redundant with request_context)
- register_routes extension point nothing passes
- version error helpers inlined into evaluate_version_policy
- unused top-level re-export facade in stitch/__init__.py

* Remove dangling cookbook artifacts

- unreachable per-request version-pin branch in the standalone slime hook
  (the hook contract never supplies rollout_id; announce_and_wait already
  gates staleness) + its helper, config knob, and tests
- megatron_r3_num_out_tokens.patch: never applied, the fix is baked inline

* Dedup the slime/miles twin adapter modules

- bulletin_hooks reads generic DELTA_APP_NAME / DELTA_SERVER_CLS_NAME env
  fallbacks; per-recipe hooks.py become pure re-export shims kept only
  because configs reference them by dotted string
- per-recipe helpers.py shrink to the real per-recipe axes (sidecar module,
  model-script attr, wake mode) + one-line wrappers; miles-only
  materialize_node_local_yaml and start_host_mem_monitor move out of the
  shared trainer_helpers into miles_disagg/helpers
- serving wrappers take the shared trainer_repo_* kwarg names; the shared
  builder computes the cookbook mount dir itself

* De-noise prose: drop provenance labels, war stories, and docstring essays

Keep the behavioral fact, drop the provenance: external PR/issue labels
(slime PR #5, radixark/miles#1261, P0.1), incident anecdotes, milestone
roadmap prose (M1/M2/M3), and layout/design essays compressed to what the
code needs the reader to know. Pinned SHAs stay; their changelog trailers go.

* Trim READMEs and packaging

- root README: document all four cookbook recipes; fix the nonexistent
  slime extra claim
- miles_disagg README: compress the fork changelog and bring-up checklist,
  fold the GLM-4.5-Air section into a variant note, point at the fork for
  the convert tools, document _spawn_into_deployed
- standalone_rollouts README: compress the compatibility spec-dump and the
  SLIME-harness tail
- slime_disagg README: merge the redundant Protocol notes into the run
  section
- pyproject: drop uvicorn from the sglang extra (packaged code never
  imports it; Modal images install it explicitly)
jvmncs added a commit that referenced this pull request Jul 20, 2026
* Explicit pool claim/reset: one trainer call owns one pool epoch


* Align reconcile tests with #4 composed-reload semantics

After rebasing onto main (which includes #4, composing a multi-version
catch-up into a single engine reload), two reconcile tests still asserted
the pre-#4 per-version apply accounting:

- local_disagg/harness_test.py (added by this PR): a cold latecomer
  catching up v1..v2 now records a single composed apply [2], not [1, 2].
- standalone_rollouts/provider_test.py (pre-existing; #4 changed the
  behavior but did not update this cookbook test): same one-apply tail.

The served version / run_id invariants are unchanged; only the per-apply
accounting moved. No production code change.
jvmncs added a commit that referenced this pull request Jul 20, 2026
* Remove dead and speculative code from the core library

- generate_rollout compatibility wrapper and the unreachable per-request
  version-pinning branch (the live hook contract never supplies rollout_id);
  the request hook now does only what runs: retry budget + session affinity
- test-only helpers: transition_artifact_paths, parse_extra_key_version,
  validate_policy (redundant with request_context)
- register_routes extension point nothing passes
- version error helpers inlined into evaluate_version_policy
- unused top-level re-export facade in stitch/__init__.py

* Remove dangling cookbook artifacts

- unreachable per-request version-pin branch in the standalone slime hook
  (the hook contract never supplies rollout_id; announce_and_wait already
  gates staleness) + its helper, config knob, and tests
- megatron_r3_num_out_tokens.patch: never applied, the fix is baked inline

* Dedup the slime/miles twin adapter modules

- bulletin_hooks reads generic DELTA_APP_NAME / DELTA_SERVER_CLS_NAME env
  fallbacks; per-recipe hooks.py become pure re-export shims kept only
  because configs reference them by dotted string
- per-recipe helpers.py shrink to the real per-recipe axes (sidecar module,
  model-script attr, wake mode) + one-line wrappers; miles-only
  materialize_node_local_yaml and start_host_mem_monitor move out of the
  shared trainer_helpers into miles_disagg/helpers
- serving wrappers take the shared trainer_repo_* kwarg names; the shared
  builder computes the cookbook mount dir itself

* De-noise prose: drop provenance labels, war stories, and docstring essays

Keep the behavioral fact, drop the provenance: external PR/issue labels
(slime PR #5, radixark/miles#1261, P0.1), incident anecdotes, milestone
roadmap prose (M1/M2/M3), and layout/design essays compressed to what the
code needs the reader to know. Pinned SHAs stay; their changelog trailers go.

* Trim READMEs and packaging

- root README: document all four cookbook recipes; fix the nonexistent
  slime extra claim
- miles_disagg README: compress the fork changelog and bring-up checklist,
  fold the GLM-4.5-Air section into a variant note, point at the fork for
  the convert tools, document _spawn_into_deployed
- standalone_rollouts README: compress the compatibility spec-dump and the
  SLIME-harness tail
- slime_disagg README: merge the redundant Protocol notes into the run
  section
- pyproject: drop uvicorn from the sglang extra (packaged code never
  imports it; Modal images install it explicitly)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant