[multi-lora] 1/n operation backend: explicit training with Tinker compatibility - #2273
[multi-lora] 1/n operation backend: explicit training with Tinker compatibility#2273yushengsu-thu wants to merge 96 commits into
Conversation
f2e60eb to
7346ed0
Compare
…ate serialization
Sample gains optional per-token float channels (loss_weights, advantages)
for client-supplied training data: response-aligned like loss_mask, merged
across turns like the OPD lists (zeros over injected observation spans),
carried on the wire as float32 typed_ragged, CP-sliced like
rollout_log_probs. The binary int32 loss_masks stay untouched.
miles/backends/megatron_utils/tinker_backend/checkpoint.py holds the slot
training-state serialization for the tinker-compatible backend: bf16
adapter weights + positional per-child optimizer state (fp32 masters, Adam
moments, step counters), per-rank atomic shards, rank-0 manifest committed
after a barrier, optional manifest ttl_seconds, and named immutable states
at states/{tag}. Loading fences on format, world topology, and LoRA
rank/alpha shape — never on the display name, so a new registration may
restore another run's state (create-from-checkpoint).
Provenance: #2242 data-channel and checkpoint commits, renamed to the
tinker namespace, minus swap-in/out (they belong to the residency layer).
d8ec792 to
a3d8038
Compare
f442377 to
f969743
Compare
Fixed residency: a registration binds the lowest free slot for its whole life or queues behind a full pool (bootstrap drains the queue at retirement); there is no eviction, no bind-at-selection, and therefore no reservation transactions — tenancy changes only on the driver-sequenced register/deregister path. Pins mark slots whose state is immovable (dirty-grads: accumulated gradients no checkpoint carries). The run lifecycle is PENDING -> READY -> RETIRING -> CLEANUP -> COMPLETED, where READY comes from the trainer finishing the slot load — never from a weight publish: serving is a separate axis (serving_version stays 0 until save_weights_for_sampler) and record_weight_update no longer promotes. commit_tinker_step advances the per-run step clock, releases the dirty pin, and honors the optional client-set num_step bound; set_step repositions the baseline for state resume. AdapterRunConfig is the client-driven minimum: rank (server ceiling --lora-rank), optional save/num_step/metadata; alpha is server-resolved and never client-settable. Provenance: #2137 slot pool/registry reworked for fixed residency and readiness/serving decoupling; #2242 tinker lifecycle methods.
…tries, strict execution order One registration is strictly serialized: an operation is claimable only when every earlier ordinal has ARRIVED and reached a terminal state, which carries the client's per-model ordering end to end and keeps an optim_step from ever overtaking its forward_backward batches. Arrival may be out of order — the tinker SDK deliberately posts the first chunk of a large forward_backward last — so operations buffer by ordinal (consecutive from 1 per registration) and a gap below the head blocks all claims until it fills. NOTE: this reorder buffer moves to the tinker frontend when one lands. Retries are fingerprinted (sha256 over kind + canonical payload): re-enqueueing a known operation_id with identical content returns the original operation; different content is a conflict error, never silently swallowed. Cancel applies to QUEUED only and the cancelled ordinal still counts for contiguity; retirement fences open operations; terminal results are retained until acked (enqueue backpressure — mapped to HTTP 429 — is the capacity knob, never result eviction). Provenance: #2242 operation ledger + the arrival/fingerprint upgrades from the design review.
…roller and HTTP surface
…t sums, all-rank NaN veto
test_rollout_data_conversion.py::test_unaligned_input_is_trimmed_to_multiple already pins the same trim-to-multiple behavior on the same production branch; keeping a second copy in the padding suite adds maintenance cost without coverage.
The abort_all_requests=False behavior was introduced on main (#2589), not by this stack, so its regression test belongs in a standalone test-only PR against main rather than riding the tinker backend; the file returns to its main-tree content.
_thread_main records a dead loop in run.error and the harness previously still exited 0 after printing the summary, so a wrapper (or a human checking $?) would read an aborted run as a pass; the summary and CSVs still land first, then the process fails if any loop aborted.
All 35 sites flagged by review 5003387723 on #2273: each multi-line comment, docstring, assert/error message, or argparse help string is now a single line keeping the load-bearing invariant; no behavior change (test-matched substrings preserved).
|
@claude review |
Unacked terminal results of a retired registration lived in the ledger forever (probe: 50 dead registrations retained 14.1 MB, monotonic). fence() now strips request payloads (fingerprints keep retry identity, results stay pollable), and evicting a COMPLETED record from the registry ring fires drop_tenant, purging the tenant's queue and by_id entries. The eviction slice also clamps at zero so under-cap rings no longer evict early. A dropped operation polls as None; the frontend already maps missing operations to typed tombstones.
…uted sync LoRA sync sends only adapter tensors and never refills base weights; opening the session anyway makes begin/end_weight_update restore and re-pack the quantized base buffers with nothing loaded in between, corrupting the frozen base (reproduced on Kimi-K2.5 W4A16, TP8). Also re-adds the update_weight_version abort_all_requests=False wire pin so main #2589's no-abort behavior cannot silently regress. Absorbed from closed PRs #2715 and #2713.
Move the import-integrity checks out of the fast suite and into tests/ci/verify_source_resolution.py, which every CPU and GPU CI job runs before pytest: statically resolve every miles-internal import site (including function-local ones) across miles/ and examples/, walk the optional namespaces in full when present, and import the update_weight lazy-import targets. Failures raise RuntimeError with the offending file:line and import target.
|
@claude review |
There was a problem hiding this comment.
I reviewed this update to the multi-LoRA operations PR and found no new bugs this run. Previously-flagged issue in this bot's own prior review — the OperationLedger memory leak for retired registrations — is now fixed: fence() (operations.py:451-455) purges queues/by_id alongside AdapterRegistry.free_slot, matching the earlier ack()-only cleanup path.
Beyond that, I spot-checked the load_state path and /adapter_runs yaml_path handling flagged as candidate issues this run: both do pass client-supplied paths to open()/restore without containment checks (unlike save_state's regex-validated tag), so that observation holds, but this control-plane HTTP server is driven by trusted internal tooling rather than arbitrary end users, which is a plausible reason it wasn't escalated. Given the size of this refactor (100+ files, new operation-contract/ledger/residency subsystems, checkpoint restore, HTTP surface) and its security-adjacent multi-tenant slot semantics, a human look remains worthwhile alongside this automated pass.
Extended reasoning...
This run's bug hunter reported no findings. This bot's own prior review on an earlier revision flagged two issues: an OperationLedger memory leak (fence/free_slot never purged queues/by_id, only ack() did) and a Dockerfile revert from a pinned Megatron-Bridge SHA to a mutable branch. I independently verified in the current diff that the ledger leak is fixed — fence() now pops queues/by_id (operations.py lines 451/455), matching commit f01778a ("bound the operation ledger by the registry's completed ring"). The Dockerfile still tracks @ bridge (not repinned to a SHA) despite the author resolving that thread without a code change, but that finding was only a nit-level build-reproducibility concern to begin with, not a functional bug, so restating it would be low value and risks being a broken record on a dismissed nit.
This run's candidate-issue list surfaced seven new items (rid/serving_name consistency, load_state/yaml_path path containment, checkpoint registration_id validation, SGLang KV-cache namespace collision, unused ttl_seconds) that were investigated and ruled out rather than reported as bugs. I spot-checked two of the path-handling ones directly in config.py/backend.py/http_server.py and confirmed the code shape described (load_state accepts any non-empty path string; yaml_path goes straight to open()/yaml.safe_load via parse_adapter_run_yaml) is accurate — no containment logic exists for either. I did not find independent evidence of exploitability given the apparent trusted-operator threat model of this control-plane service, consistent with the sub-agents' decision not to escalate these as bugs.
Given the PR's scope (a 100+ file refactor introducing new operation-contract, ledger, residency, and executor subsystems plus a service HTTP surface with client-supplied paths and checkpoint restore across multi-tenant LoRA slots) and that this is not a mechanical or narrow change, the appropriate level of scrutiny remains high, warranting continued human review even though no new bug was found this run and the previously-identified real defect has been fixed.
…bridge The launch-time probe (_bridge_recompute_patch_recognizes_multi_lora and its source-inspection helper) rejected full recompute and expert-target MoE recompute on a Megatron-Bridge without #27. The deployment now tracks the bridge branch, which carries #27, so the pre-#27 shape can no longer reach launch; the guard and its test are retired. The CI-level LayerWise dependency canary remains the guard against a stale image. README and docs mirror drop the bridge-version requirement wording; supported recompute combos stay documented.
Absorbs the recently merged split PRs and maintainer changes. One conflict: miles/backends/megatron_utils/multi_lora_optimizer.py was modified on main (Megatron-LM bump renamed enable_gloo_process_groups to use_gloo_process_groups) but the stack deletes the legacy adapter-sample-level path entirely; resolved as deleted, and the same rename is applied to the stack's replacement (api_backends/multi_lora/optimizer.py and its test) so it matches the bumped Megatron argument name.
Summary
Replaces the old dataset-driven Multi-LoRA path with a service-only, client-driven operation backend. Multiple clients share one Megatron base model through fixed LoRA slots and explicitly control forward/backward, optimizer, checkpoint, and sampler-publication boundaries.
This PR provides the backend and Ray-side operation contract. Stacked PR #2346 adds the official
tinker==0.24.1JSON/REST frontend, and #2365 adds the complete client-owned SFT/RL guide.Reviewer
MultiLoraParameterExecutoris deliberately narrower than the whole backend: it owns optimizer step/discard for lease-bound adapter slots. Data conversion and forward/backward live inMultiLoraOperationBatchFn; checkpoint and publication remain Multi-LoRA trainer controls.create_rollout_components()also exposes separateInferenceControllerPortandRolloutExecutorPortroles. Today they are two adapters over the same combinedRolloutManager; the physical controller/executor split remains future integration work.Operation semantics
Supported operations are:
forwardforward_backwardoptim_stepsave_weights_for_samplersave_stateload_stateImportant invariants:
(name, registration_id)prevents stale handles from targeting a re-registered adapter with the same display name.forward_backwardcalls accumulate one client-owned gradient window. A failed batch poisons that window, and the nextoptim_stepdiscards it instead of applying partial gradients.Sampling, scoring, rewards, advantages, batch scheduling, and SDK
Datumconstruction remain client-owned.
Full-parameter seam
The reusable seam is intentionally narrower than the complete backend:
BatchExecutionLeaseTrainerResidencyPortParameterExecutorrun_optim_controlsFullParameterExecutorimplements singleton whole-model Adam step/discardbehavior against a stock Megatron optimizer. It is not connected to launch
configuration, registration, data conversion, forward/backward execution,
checkpointing, controller routing, or sampler publication. This PR therefore
does not claim working full-parameter SFT/RL or full-parameter GPU/E2E support.
Validation
Current head
7189b1e54has:pending checks.
multi-adapter training/checkpointing, sampler publication, restore,
registration fencing, and slot reuse.
Limits
cross_entropy,importance_sampling, orppo.qkv_formatmust bethd.optimizer ownership.
guaranteed; executor-side reconciliation remains future work.
Stack and dependencies
tinker==0.24.1JSON/REST frontend.miles-main.support; merged into the tracked
bridgebranch.0821 liveness fixes: capped tolerance for consecutive generate failures (
--multi-lora-max-consecutive-generate-failures), a claimed-operation TTL backstop that unblocks orphaned CLAIMED queue heads (--tinker-operation-claimed-ttl), and FAILED child runtimes self-healing to IDLE after a cooldown.0821: fixed the publish-path function-local import stranded by the api_backends regrouping (
update_weight .../mixin.py→api_backends.multi_lora.model) +tests/fast/test_import_integrity.pystatic import-integrity regression; 2-GPU mini-loop re-verified publish+sample (PASS).0822 test cleanup: launch-unreachable multi-LoRA tests removed (stamped-slot fallback deleted from production, now a loud ValueError), duplicate trim test dropped, sglang abort regression moved to its own PR [multi-lora] 1/n - 7, test: pin update_weight_version abort_all_requests=False regression #2713, RL-quality harness now exits non-zero on aborted loops.