Repository navigation
[multi-lora v2 6/7] Tinker gateway runtime and entry point - #2845
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.
61830bc to
5949c6e
Compare
c05d86c to
b460abd
Compare
5949c6e to
16085f5
Compare
b460abd to
9078f69
Compare
16085f5 to
afacac6
Compare
9078f69 to
cd5c610
Compare
afacac6 to
06e19ce
Compare
1a0788f to
b1368bc
Compare
06e19ce to
0f8c267
Compare
5a82719 to
1fd3a27
Compare
0f8c267 to
dd41980
Compare
88fd1e3 to
e8302d3
Compare
d7cb98f to
cf780a5
Compare
cbbc358 to
4fc45b2
Compare
9b9c0b1 to
81c2753
Compare
940c322 to
0032696
Compare
81c2753 to
a96d38f
Compare
0032696 to
8c93e05
Compare
a96d38f to
6d1a22f
Compare
8c93e05 to
976bbdd
Compare
6d1a22f to
8bd4918
Compare
976bbdd to
f1ea9d1
Compare
8bd4918 to
2a5cd28
Compare
f1ea9d1 to
0168b18
Compare
23c71ff to
ec181c4
Compare
b470e76 to
0b2e616
Compare
ec181c4 to
72b0a08
Compare
0b2e616 to
19fba08
Compare
72b0a08 to
0b2e616
Compare
19fba08 to
fb201e7
Compare
fc48382 to
3f5da19
Compare
fb201e7 to
2f6896b
Compare
nanjiangwill
left a comment
There was a problem hiding this comment.
Runtime integration review: three correctness/availability concerns and two explicitly non-blocking lifecycle suggestions.
| logger.info(f"tinker gateway serving {config.base_model} on :{args.tinker_server_port}") | ||
| service_task = asyncio.create_task(service.run()) | ||
| try: | ||
| await server.serve() |
There was a problem hiding this comment.
Addressed: service.run() and Uvicorn are now supervised together, and completion or failure of either task tears down the other.
| store = object_store.get_instance() | ||
| data_ref = store.put(value=train_data, value_spec=ROLLOUT_DATA_VALUE_SPEC) | ||
| try: | ||
| return await getattr(self.trainer, method)(batch_id=batch_id, data_ref=data_ref) |
There was a problem hiding this comment.
TrainerController._execute_slots() calls TrainerCell.execute() with the default kill_on_failure=True; any actor exception marks the only trainer cell errored and kills its workers. TinkerService instead treats batch failures as recoverable and calls zero_grads() before continuing, so cleanup targets a dead cell and can stop dispatch. Create and load failures likewise leave the service alive with no usable trainer. Align the contracts: either stop the gateway when a trainer RPC kills the cell, or classify recoverable request errors before that boundary. Keep CUDA/NCCL failures cell-fatal.
| "sequence_id": f"seq-{uuid.uuid4().hex}", | ||
| "tokens": [entry[1] for entry in output_token_logprobs], | ||
| "logprobs": [entry[0] for entry in output_token_logprobs], | ||
| "stop_reason": "length" if finish == "length" else "stop", |
There was a problem hiding this comment.
_to_sequence() maps every finish reason except length to stop. SGLang returns type="abort", and miles’ normal rollout path maps that to Sample.Status.ABORTED; here it becomes a successful partial sample that the client can train on. Raise on abort—the Tinker wire enum has only stop and length—so _run_sample() settles the request as a server failure.
| ) | ||
| await trainer.init() | ||
|
|
||
| checkpoint_root = args.tinker_checkpoint_root or (args.save and f"{args.save}/tinker") |
There was a problem hiding this comment.
Non-blocking: compute and validate checkpoint_root before launch_worker_manager() and controller initialization. A missing --tinker-checkpoint-root/--save currently starts the worker and GPU stack before hitting this assertion.
| # service.run() loops forever; cancel it so the gateway can exit | ||
| service_task.cancel() | ||
| with suppress(asyncio.CancelledError): | ||
| await service_task |
There was a problem hiding this comment.
Non-blocking: dispose trainer and inference_controller in this finally block. Their disposal stops watchers, tickers, and inference server clients; cancelling only service_task leaves those resources live until process exit.
Co-authored with @yushengsu-thu.
Connect the protocol service to Miles through
MilesBackendandserve_tinker.py.Training commands place a work unit in the object store, let workers split it, and reassemble per-datum outputs by sample index. Forward-only runs the requested loss without backward. Explicit actor failure results pass through to the gateway; unknown trainer failures retain the common
TrainerCellboundary and terminate the current single-cell service through its supervisor.The entry point launches the worker manager, inference controller, trainer, service loop, and HTTP server. It requires the trainer to load the same frozen HF checkpoint as the inference engines. The runtime sends sampling requests through the router with the immutable adapter name and disk backfill path, without owning an inference controller or a weight-update window. Save and export commands forward gateway metadata to the worker's directory transaction. Checkpoint execution errors propagate through the same trainer-cell and supervisor shutdown path.
The gateway exposes its port, base-model name, and checkpoint root, and derives its adapter layout from normalized HF target names. Both standard and canonical Megatron target names are supported. Per-slot optimizer ownership is established in #2840.
Stacked on #2844.
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
Controller lifecycle: match
train.pyby disposing the inference controller, then the trainer, after normal completion of the serving tasks. NoAsyncExitStackor initialization/exception cleanup guarantees are introduced. Full pre-commit passed; no regression tests were added.Router dependency: install the router wheel and standalone binary from the existing
cu130-torch213-x86_64/cu130-torch213-aarch64rolling releases inyueming-yuan/miles-wheels, using the normal/tmp/wheelsinstallation path. Both architectures were rebuilt on CPU devboxes fromradixark/sgl-router-for-miles@b6342a3515e259bbd1eb4db728035a4efbe4fdf9and verified to forwardreturn_indexer_topkandlora_backfill_paths. No commit-specific router release is required. The existingpr-2849Docker image has not been rebuilt by this change.Router-pin validation: full pre-commit passed; the Docker RUN shell syntax and image-input hash change were checked. Both architectures' wheel and binary downloads match the four previously tested SHA256 digests, and the source checkout resolves to the exact pinned commit. The existing native HTTP probes passed for both artifacts on both architectures. No new tests were added. The new Miles PR image and hosted CI have not finished yet.