Repository navigation
[multi-lora v2 5/7] Tinker gateway protocol face - #2844
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
a89d49c to
3516002
Compare
61830bc to
5949c6e
Compare
3516002 to
470cce0
Compare
16085f5 to
afacac6
Compare
|
Planner rewritten to the final architecture (no interim FCFS-passthrough): per-model streams compile command runs into commutative windows; the planner packs ready datums across models into WorkUnits by (kind, loss_fn, config) equivalence class under a token budget — cross-tenant packing and large-request splitting are the same datum-granular bin-packer; ready optim barriers of different models merge into one BarrierUnit ( In-memory fake-executor smoke covers packing, barrier merge, seq ordering across barriers, out-of-order arrival, idempotent resubmit, and tenant ownership. |
06e19ce to
0f8c267
Compare
470cce0 to
5b41a4d
Compare
0f8c267 to
dd41980
Compare
5b41a4d to
d0b22ae
Compare
d7cb98f to
cf780a5
Compare
d0b22ae to
bc6d9bc
Compare
c3fbd7f to
0c6bd91
Compare
bc6d9bc to
4554c4d
Compare
0c6bd91 to
389213a
Compare
4554c4d to
fcd9d7c
Compare
389213a to
11d271a
Compare
fcd9d7c to
05bdcec
Compare
cac5dbe to
7f72e4f
Compare
0b4d278 to
8d14348
Compare
373a4c1 to
68c5301
Compare
8d14348 to
602fab6
Compare
68c5301 to
dcce64a
Compare
602fab6 to
8428765
Compare
dcce64a to
ab91e2b
Compare
99f16f2 to
1cb92ba
Compare
ab91e2b to
20e88a7
Compare
|
|
||
| def create_model(self, tenant: str, payload: dict) -> tuple[str, str]: | ||
| """Two-phase like every command: allocate now, initialize the slot behind the future.""" | ||
| base_model = payload["base_model"] |
There was a problem hiding this comment.
CreateModelRequest includes session_id and model_seq_id, but this method ignores both and always allocates a random model and slot. Tinker 0.26.2’s generated POST client retries up to 10 times by default with the same request body, so a lost response can run load_slot() twice and consume two slots. The model is also leased by tenant, so another live session for the same API key keeps it alive. Validate the session, key creation by (tenant, session_id, model_seq_id), store the owning session, and return the existing future/model on retry.
| except Exception as error: | ||
| self.models.pop(record.model_id, None) | ||
| self.planner.remove_stream(record.model_id) | ||
| self.free_slots.add(record.slot) |
There was a problem hiding this comment.
load_slot() can fail after init_adapter_slot, gradient reset, or optimizer reset has already mutated some ranks, but this exception path returns the slot directly to free_slots. The test fails before any mutation, so it does not cover that state. Run unload_slot() before reuse and return the slot only if cleanup succeeds; otherwise quarantine it.
| sampling_session_id = f"sampling-{uuid.uuid4().hex}" | ||
| self.sampling_sessions[sampling_session_id] = { | ||
| "tenant": tenant, | ||
| "model_path": payload.get("model_path"), |
There was a problem hiding this comment.
CreateSamplingSessionRequest carries session_id, sampling_session_seq_id, and a base_model/model_path binding, but this stores only model_path. Later SDK sample requests normally send only sampling_session_id, so a session created with base_model="other" is accepted and then samples this gateway’s configured base model. The stable (session_id, sampling_session_seq_id) is also ignored, so a retry creates another session. Validate the parent session and model selection, store the full binding, and deduplicate creation by that pair.
| f"{self.config.max_samples_per_request}" | ||
| ) | ||
| lora_name, lora_path = self._resolve_sampler(tenant, model_path) if model_path else (None, None) | ||
| future = self.futures.create(model_path or "base", tenant) |
There was a problem hiding this comment.
decode_sample_request() drops seq_id, and submit_sample() always creates a new future and task. Tinker 0.26.2 calls execute_with_retries() with the same request ID, so a lost response runs generation again and returns different IDs. Preserve seq_id, deduplicate by (sampling_session_id, seq_id) before scheduling, return the retained result on retry, and fail terminally after retention expires.
| raise UserInputError(f"unknown command op {op!r}") from None | ||
| model_id = payload["model_id"] | ||
| self.get_model(tenant, model_id) | ||
| seq_id = payload["seq_id"] |
There was a problem hiding this comment.
Non-blocking: ModelStream advances only from watermark 0 to sequence 1, but submit() accepts seq_id <= 0 and non-integers. Those entries can never match watermark + 1, so their futures remain pending forever. Reject non-integers and values below 1 before creating the future; protobuf’s missing value is 0, so cover that boundary.
|
|
||
| async def _fail_if_poisoned(self, stream, pending) -> bool: | ||
| """Fail the next optimizer step when a batch discarded its accumulated gradients.""" | ||
| poison = self._poisoned_slots.pop(stream.slot, None) |
There was a problem hiding this comment.
_poisoned_slots.pop() runs before zero_grads(). If zero_grads() raises, _run_barrier() fails the current optimizer request, but the poison has already been forgotten; the next optim_step can run on ambiguous gradients. Keep the poison until cleanup succeeds. If cleanup fails, retire or quarantine the model instead of continuing to serve it.
| await self._retire_after_failed_step(stream, pending, f"{type(error).__name__}: {error}") | ||
| return | ||
| for stream, pending in entries: | ||
| outcome = outcomes[stream.slot] |
There was a problem hiding this comment.
Non-blocking: validate the complete outcome map before settling any future. This loop can resolve and remove one stream, then hit a missing or malformed outcome for a later slot. _run_barrier() then calls _fail_barrier() for every entry, and the second stream.finish() on the already removed entry raises ValueError, terminating service.run(). A validation pass before settlement avoids the partial state.
| "seq_id": message.seq_id, | ||
| "datums": datums, | ||
| "loss_fn": message.loss_fn, | ||
| "loss_fn_config": dict(message.loss_fn_config), |
There was a problem hiding this comment.
Non-blocking compatibility note: this decodes only the legacy loss_fn_config. Tinker 0.26.2 dual-writes numeric values to both maps but writes strings only to loss_fn_config_v2, so string settings are silently dropped. Prefer loss_fn_config_v2 when present and fall back to the legacy map.
Co-authored with @yushengsu-thu.
Add the gateway's protocol and scheduling layers under
miles/tinker/coreandmiles/tinker/server.X-API-Keyand bearer tokens.A sampler save exports the current adapter and returns its path after the immutable directory commits. It makes no inference-engine calls and neither applies nor discards accumulated gradients. Sampling resolves ownership and base-model compatibility from the saved metadata whether or not the training model still exists. There is no separate in-memory publication registry.
Known request-local forward-only and sampling failures leave the training stream available. Checkpoint load/save/export execution failures terminate the shared trainer cell and service;
load_staterejectssampler_weightsbefore metadata lookup or backend execution because the training loader requires Megatron DCP.weights_inforemains available for both checkpoint kinds. Failed sampling does not invalidate a saved snapshot. Unknown trainer and background-task failures reach the serving supervisor. This implements failure handling and isolation; automatic recovery and stream replay are not implemented.Model creation rechecks its record under the trainer lock. Model closure fails its creation future and only unloads initialized slots. Creation, training, and sampling duplicates whose results expired receive a stable terminal failure without replaying backend work. Unknown sampling sessions are user errors; foreign-tenant sessions remain ownership errors.
Futures, sessions, leases, deduplication, and unsaved accumulation are process-local. Saved checkpoints retain ownership and adapter metadata. These protocol layers do not import
miles.rayormiles.backends; runtime wiring follows in #2845.Request completion is centralized in
_finish_request; batch and barrier execution share its failure policy. Checkpoint helpers live incore/utils.py, request validation stays in the service, and retry-result retention belongs toFutureStore. The dispatcher cancels and awaits its model-creation, sampling, and lease tasks before exiting.Saving preserves the public checkpoint link; loading resolves it once before metadata validation and backend execution. Legacy checkpoints can be loaded, while legacy-directory overwrites are rejected before reaching the trainer.
Stacked on #2843.
Validation:
pre-commit run --all-filesand incrementalgit diff --checkpassed on every updated component head. All previous heads remain ancestors; the final Unify HF snapshot and checkpoint writing #3198 tree exactly matches the reviewed implementation.tests/e2e/lora/test_slot_checkpoint_resume.py(Hopper, LoRA, 2-GPU stage). The tested final tree is Unify HF snapshot and checkpoint writing #3198fd63c7ec8c41c2e0a61d273f0cd5a3504275ada7, with Megatron73b54618f7e58e0f25f619bcfecbe2640765475a, Bridge40b930897717941cfe2bd9806f417e28bf1bfa65, PyTorch 2.13.0+cu130 and Transformer Engine 2.17.0. Checkpoints were on GPFS.pre-commit run --all-filespassed.ci-megatron-pr: 73b54618f7e58e0f25f619bcfecbe2640765475a
ci-sglang-pr: 02b5e123c490c988d21526d0dd7837fefdd0e1da
Session lease correction: models, sampling sessions, and in-flight sampling requests track their owning session. Expiry reclaims only that session’s resources; direct sessionless sampling is unaffected. The sweep decides expiry after acquiring the trainer lock and settles sampling futures before cancellation.
Validation: a real-service/fake-backend before/after probe reproduced stale model retention and confirmed selective slot reclamation. No regression tests are added for this change. Full pre-commit passed; CPU CI execution is pending.
Protobuf loss configuration: prefer v2 number/text values when present, otherwise decode the legacy numeric map. SDK 0.26.2 protobuf probes cover legacy, v2 precedence, v2-only values, zero, empty text, and malformed values. Full pre-commit passed; no regression tests were added.