Split RolloutManager into InferenceController and RolloutExecutor - #1842
Open
fzyzcjy wants to merge 1 commit into
Open
Split RolloutManager into InferenceController and RolloutExecutor#1842fzyzcjy wants to merge 1 commit into
fzyzcjy wants to merge 1 commit into
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-1
branch
from
August 3, 2026 23:39
b140598 to
0887196
Compare
fzyzcjy
requested review from
Shi-Dong,
Ying1123,
Zhichenzzz,
guapisolo,
jybsuper and
yushengsu-thu
as code owners
August 3, 2026 23:39
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-8
branch
from
August 3, 2026 23:44
5d9d66a to
e8d6992
Compare
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-1
branch
from
August 4, 2026 06:56
0887196 to
5f089d6
Compare
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-8
branch
from
August 4, 2026 06:56
e8d6992 to
0478695
Compare
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-1
branch
from
August 10, 2026 06:06
5f089d6 to
1a71cad
Compare
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-8
branch
from
August 10, 2026 06:06
0478695 to
98fbe94
Compare
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-1
branch
2 times, most recently
from
August 10, 2026 16:34
cc79ac3 to
8dc50ea
Compare
yushengsu-thu
added a commit
to yushengsu-thu/miles
that referenced
this pull request
Aug 12, 2026
…s own HTTP plumbing; no behavior change TinkerBackend discovered router workers and posted aborts itself (worker_urls + abort_adapter_requests over its own httpx client), so the engine lifecycle owner could never change under it (codex-rollout-fullparameter-design-0810 §4.6/§4.2). miles/ray/tinker_backend/inference_admin.py: the InferenceAdminPort protocol (abort_registration by registration-scoped rid prefix — the anti-ABA namespace) and RouterInferenceAdmin, the current adapter with the exact same /list_workers|/workers discovery, /abort_request posts, timeouts, and warnings (pure move). The backend delegates abort_adapter_requests to the port and its httpx client moves with the machinery; a post-PR-radixark#1842 adapter delegates to the InferenceController instead. Registry state, serving versions, and sampling-session authority stay in the backend — none of that moves behind the port. Deleted as superseded: TinkerBackend.worker_urls and the backend-owned httpx client (both had no consumer outside the abort path).
yushengsu-thu
added a commit
that referenced
this pull request
Aug 12, 2026
…tinker batch_plan key; no behavior change RolloutManager._get_rollout_data decided DP padding by probing the fn's metadata for a "batch_plan" key and imported batch_plan_to_metadata to convert it — the one place the generic rollout plane had to recognize a tinker-specific control plane. RolloutFnTrainOutput now carries typed RolloutPostprocessOptions (pad_to_dp) plus an opaque conversion_metadata contribution that the manager merges verbatim. TinkerRolloutFn declares pad_to_dp=True and ships its BatchPlan already converted (batch_plan_to_metadata moves to miles/rollout/tinker_backend/rollout_fn.py, next to its only producer). The existing fn-internal metadata field keeps its old meaning and is now ignored by the manager, so custom rollout fns see no change either. Equivalence: batch_plan_to_metadata is a pure move (body untouched, the pre-existing TestBatchPlanToMetadata characterization carries over); pad_to_dp was true exactly when batch_plan was present, i.e. exactly for TinkerRolloutFn outputs, which now declare it; no in-tree fn other than tinker ever set output metadata, so the verbatim merge adds nothing. test_rollout_fn captures the pre-refactor manager composition byte-for-byte, and the new manager test proves the typed flag reaches postprocess (7 samples pad to 8 with the -1 sentinel row). This also removes the RolloutManager coupling PR #1842 would trip over (the future RolloutExecutor inherits the contract unchanged), without implementing any of the split itself. (codex-rollout-fullparameter-design-0810 §4.4)
yushengsu-thu
added a commit
that referenced
this pull request
Aug 12, 2026
…— call sites adopt the PR #1842 boundary today; no behavior change The driver consumed one combined RolloutManager handle for three roles at once — router endpoint discovery (inference ownership), operation batch generation (execution), and lifecycle — the exact seam PR #1842 will split (codex-rollout-fullparameter-design-0810 §4.3/§4.7/§4.8; #1842 is still open, so this builds our side of the boundary only). miles/ray/rollout/components.py fixes the consumer-facing names NOW: InferenceControllerPort / RolloutExecutorPort / RolloutLifecyclePort carry only what the driver needs, RolloutComponents bundles them with num_rollout_per_epoch and exactly-once disposal, and create_rollout_components() is the ONE construction seam. The current concretes are Legacy...Adapter views over the same combined actor (deliberately not the future class names — no import collision — and not _tbd: Legacy states what the object is and when it dies). The combined handle stays reachable on the inference-owner adapter for the engine/weight-update plumbing that create_training_models still wires into the training actors — after the split, the real controller owns that wiring. train_tinker_backend.py now speaks in roles: inference_controller resolves the InferenceEndpoint (router host/port for sampling and the control API), rollout_executor.generate() runs the data phase, the bundle disposes once. The physical publish barrier is wrapped in the parameterless ActorGroupWeightPublisher.publish_staged_weights() — no operation IDs, no lease, no second binding list; the actor keeps sole authority over pending-push coalescing, the has_new_engines trigger, and resident push-set selection, so new-engine recovery without control operations needs no synthetic lease. Explicitly NOT done, per the doc's defers: no copy of #1842's classes, no RolloutManager.tinker_generate(), sampling never routes through the executor, and the control-first ordering + publish barrier are untouched (the driver's call trace test pins reconcile -> controls -> publish -> generate/train). Contract tests (tests/fast/ray/rollout/test_components.py): the factory unpacks (manager, num_rollout_per_epoch), returns two distinct role objects over one shared handle, disposes exactly once, and future-shaped fakes satisfy the bundle without touching driver call sites; the module imports no Ray.
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-1
branch
from
August 12, 2026 14:31
8dc50ea to
fb28f27
Compare
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-8
branch
from
August 12, 2026 14:31
4c17d0c to
ad66bc8
Compare
yushengsu-thu
added a commit
that referenced
this pull request
Aug 12, 2026
…dow consumption, exact identity, lease atomicity, port boundaries Green happy-path/equivalence tests hid several failure-path bugs (16-test adversarial suite: 15 failed at the #2365 head). The backend-side fixes: - optimizer outcomes now carry gradient_window_consumed, set only by a step, a successful discard, or a veto that zeroed the grads on every rank. A pre-mutation executor refusal (stale binding, missing result) no longer clears the dirty pin or delimits the ledger's poison window — the partial gradients still physically exist, and the next optim_step must still be routed to a discard. The ledger delimiter now requires the consumed mark (Operation.window_consumed), not mere claimed+terminal. - run_optim_controls fails CLOSED on a missing executor outcome (server error, nothing consumed) instead of defaulting a missing discard to ok and booking the user-poison terminal over an untouched window. - MultiLoraParameterExecutor refuses duplicate physical step targets deterministically (both operations get explicit server errors, no mutation) instead of rekeying by slot and silently dropping one. - _adapter_slots_from_lease validates the sample's FULL registration identity (name AND registration_id) against the lease binding — the anti-ABA case where a stale Datum of a re-registered name could route onto the same-name successor's slot — and requires unique lane operation ids with exact set equality against the lease bindings. - _execute_state_op validates the complete (name, registration, slot) tuple: an operation whose lease binding names another tenant is refused before any storage/publish mutation. - TinkerOperationBatchAdapter._merge builds the merged batch without mutating the selected runtimes and consumes their outputs only after residency.acquire_batch succeeds: a failed acquisition returns the runtimes to READY with outputs intact (the CLAIMED operation stays retryable) instead of orphaning the only in-memory copy. - InferenceAdminPort declares the init()/close() lifecycle the backend actually invokes, so a fake implementing the declared protocol works. - InferenceControllerPort gains prepare_rollout() (the PR #1842 controller responsibility); the driver calls it before every generate, reaches the training weight-update target only through the factory's opaque weight_update_owner, and the legacy adapter no longer leaks .manager. The adversarial suite's assertions are absorbed as permanent regressions in the corresponding test modules.
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-1
branch
from
August 13, 2026 06:01
fb28f27 to
ee9ca13
Compare
Squashed from: - mv - fmt - Import asyncio in the inference controller - rename - rename cls - Move the _server property with the code that uses it - Move the rollout lifecycle hooks with the state they touch
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-8
branch
from
August 14, 2026 04:45
280136d to
f038254
Compare
fzyzcjy
force-pushed
the
tom/refactor-miles/op2-1
branch
from
August 14, 2026 04:46
ee9ca13 to
04b9e93
Compare
yushengsu-thu
added a commit
that referenced
this pull request
Aug 14, 2026
…places manager-side tinker identity reconstruction External review 0813, blocking findings 4.1/4.4/4.8. The orphan window: once TinkerOperationBatchAdapter._merge() returned a leased selection, any manager failure before generate() returned (debug save, logging, conversion, DP split, object-store placement) lost the only driver-visible finalization receipt — the claimed operations stayed CLAIMED forever, blocking their registration streams, and the lease release hook never ran. - base_types: add opaque RolloutFnHandoff (fn-to-driver dispatch sidecar, same species as RolloutPostprocessOptions) + the optional RolloutFnHandoffAborter capability; RolloutFnTrainOutput carries it. - adapter: _merge() mints the handoff ONCE, where the operation ids and lease are exactly known; abort_handoff() terminal-fails exactly those operations and releases exactly that lease through one idempotent controller boundary (fail_tinker_batch fails only still-CLAIMED ops, releases in finally, so a repeat or a race with the driver's train finalizer cannot overwrite a landed result). New BatchAbortPort keeps the adapter Ray-free. - manager: the whole downstream phase (postprocess through store placement) is wrapped; on any exception or cancellation the handoff abort runs shielded and awaited-to-completion before the original failure re-raises; an abort failure is logged, never raised in place of the original. The manager forwards driver_metadata verbatim as rollout_fn_metadata and owns no tinker name anymore: tinker_dispatch_summary() (reconstruction of dispatch identity from converted tensors) is deleted. - validate_tinker_args: reject custom_convert_samples_to_train_data_path, load_debug_rollout_data, and ci_inject_rollout_data_path in tinker mode — each replaces/bypasses the live rollout output, so a dispatched batch would carry lane maps/lease that do not describe the current claim (finding 4.4; the custom-converter path silently erased dispatch identity after claim). - driver: reads the opaque rollout_fn_metadata sidecar; the driver, not the manager, interprets it as tinker dispatch identity. Regressions absorbed from the review's adversarial suite (tests 1-3) and probes P1a/P1b/P4a/P4b, rewritten to assert the FIXED behavior: downstream failures (save/convert/split/postprocess) abort the exact operations + lease, abort failures never mask the original error, duplicate finalization is idempotent, the handoff survives conversion/DP-split/delayed-store, and the manager module carries no tinker identity. Known non-goals (unchanged, documented): failures before the output receipt, process death, and repeated cancellation during the abort itself need controller-side reconciliation — PR #1842 executor scope.
guapisolo
approved these changes
Aug 22, 2026
guapisolo
left a comment
guapisolo
left a comment
Collaborator
There was a problem hiding this comment.
w/ 1843 the split is complete
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
2 participants
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.
Part of #1837