Skip to content

[tinker] 13/n towards Kimi K2.6: adapter-only colocated weight sync for Tinker API LoRA (merge_lora=false) - #2064

Open
casper-hansen wants to merge 12 commits into
NovaSky-AI:mainfrom
casper-hansen:casper/kimi-13-adapter-only-sync
Open

[tinker] 13/n towards Kimi K2.6: adapter-only colocated weight sync for Tinker API LoRA (merge_lora=false)#2064
casper-hansen wants to merge 12 commits into
NovaSky-AI:mainfrom
casper-hansen:casper/kimi-13-adapter-only-sync

Conversation

@casper-hansen

Copy link
Copy Markdown
Contributor

Part of the Kimi K2.6/K2.7 series. Stacked on #2026 (4/n), #2031 (9/n), #2062 (11/n), and #2063 (12/n) — their commits appear in this diff; review the last commit ("Adapter-only colocated weight sync for megatron LoRA") for this PR's own changes.

What

save_weights_for_sampler routes megatron + LoRA + merge_lora=false colocated runs through a new adapter-only path instead of the full-weight NCCL broadcast:

  1. Collective adapter export + per-node write via a new worker method save_lora_adapters (the engine-load half is split out of the worker's fused save+load with send_load_request=False). The bridge export reads only the LoRA adapter tensors, which stay GPU-resident through offload ([megatron] 11/n towards Kimi K2.6: file-backed offload of frozen masters so Tinker API LoRA training fits host RAM #2062 exempts the LoRA DDP buffers), so the TB-scale frozen masters remain offloaded the whole time.
  2. Offload masters/optimizer only if a preceding forward/optim phase left them resident, so the engines fit on the GPUs; a cold sync skips this entirely.
  3. Wake the engines only if they were asleep (engines_asleep is threaded from the backend). The eager post-create sleep is also skipped on this path — it was a pure sleep→wake round-trip.
  4. Reset the prefix cache and issue the adapter load from the engine process once the engines are awake.

Supporting changes: _prepare_for_weight_sync releases cached CUDA allocator blocks before engine wake (also benefits the classic path); persist=True sampler saves on this path tar the just-written PEFT adapter files instead of running a merged multi-TB save_hf_model export (the classic persist path keeps the HF export but sleeps the engines first); SKYRL_LORA_SLEEP_LEVEL lets TB-scale LoRA runs opt into level-2 (discard-and-reload) sleeps. The classic full-broadcast path is unchanged for FFT / merged-LoRA. Unit tests cover the cold/hot/wake orderings (test_worker_dispatch.py).

Why

With merge_lora=false, vLLM serves the released base checkpoint and hot-loads the adapter — yet the colocated sync still moved terabytes to deliver gigabytes: backload ~1.19TB of frozen masters to GPU, wake engine weights, broadcast, offload again, wake KV cache.

Measured on our Kimi K2.7-Code runs (2x8xB300, INT4 QAT + LoRA rank 32 through the Tinker API): a sync took 13–80 minutes when it survived at all. Run-start syncs re-faulted TBs of mmap'd frozen weights through a page cache the engine build had just evicted (554GB checkpoint read + ~830GB/node sleep backup), stalling save_weights_for_sampler 15–25 minutes — past the client's deadline, which abandoned and retried, rebuilding the engines and repeating the eviction in a loop that never converged. The eager post-create sleep alone cost 5–6 minutes of ~830GB/node CPU backup (sequential ~50s/worker) for engines that the first sync would immediately wake. And the trainer's freed-but-cached allocator blocks (tens of GB after a forward/optim step) pushed wake_up(tags=["weights"]) over GPU capacity.

With this path, a cold sampler sync exports + loads in seconds with the engines awake the whole time, and the first end-to-end Kimi K2.7 LoRA training run through the Tinker API completed with stable step times.

Made with Cursor

casper-hansen and others added 12 commits August 13, 2026 08:08
…viable for very large MoE

On a 384-expert / 61-layer model (Kimi-K2.7-Code) the first LoRA disk
sync OOM-killed workers node by node; _save_lora_adapters_and_sync had
three compounding costs:

- Only rank 0 wrote the PEFT files, which requires a shared filesystem:
  with merge_lora=false every vLLM worker hot-loads the adapter from its
  *local* filesystem. The first rank of each node now writes (identical
  content, atomic renames so engines never observe partial files), and
  rank 0 sends the LoraLoadRequest after a barrier.
- Every rank kept a full copy of the gathered adapter state; with
  per-expert PEFT keys that is tens of GB per rank (~650 GB per node in
  aggregate). Only the per-node writer ranks materialize it now -- the
  others just participate in the collectives.
- The float32 upcast doubled the state and the on-disk adapter for no
  fidelity gain (adapters train in bf16 and vLLM casts on load). The
  export keeps the training dtype.

Also expose megatron-bridge's capacity-normalized MoE LoRA
(megatron_config.lora_config.normalize_moe_lora: expert rank =
rank/topk); at full rank the per-expert export is inherently huge (41 GB
at rank 32) and is rewritten + re-read by every engine each sync (~5 GB
with normalization).

Co-authored-by: Cursor <cursoragent@cursor.com>
…ine wake/offload

Four fixes that make sampling work reliably on the SkyRL-Train backend
outside the train->save_weights->sample happy path:

- create_sampling_client(base_model=...) maps to model_id "" on the API
  side, but sample() validated every model_id against the registered
  adapters and rejected "" as unknown. Treat falsy model_ids as
  base-model requests.
- Under LoRA weight sync (megatron + merge_lora=false),
  resolve_policy_model_name() returns the skyrl-lora adapter alias, so
  base-model sampling 404'd on vLLM: the alias only exists after the
  first sampler-weight save, and applying adapter deltas to a base-model
  request would be wrong anyway. Resolve falsy model_ids to
  generator.inference_engine.served_model_name / the policy model path.
- Colocated engines are slept right after init and around every training
  op, and only save_weights_for_sampler woke them -- so a cold sample
  (base model, or an already-synced adapter) queued against sleeping
  engines and hung forever. Track engine sleep state on the backend,
  wake (weights + KV cache) on the sample path after offloading any
  GPU-resident trainer via the new WorkerDispatch.offload_for_sampling,
  and normalize to the asleep state before save_weights_for_sampler's
  wake->broadcast->wake dance.
- Lazy engine bring-up runs on the first sampling-related call, which in
  a multi-tenant service can land right after another tenant's
  forward/forward_backward left the trainer GPU-resident; under
  colocate_all the engines' startup allocation then fails ("Engine core
  initialization failed"). Offload the trainer first, matching the build
  path's build -> offload -> engines order.

Co-authored-by: Cursor <cursoragent@cursor.com>
The flag lives on the shared MegatronLoraConfig and megatron-bridge
supports it on both PEFT types; lora_type=canonical_lora silently kept
full expert rank.

Co-authored-by: Cursor <cursoragent@cursor.com>
…engines

offload_for_sampling only offloaded the named role (callers passed
"policy"), so a preceding critic forward/forward_backward left the
critic GPU-resident on the cold-sample and lazy engine bring-up paths
and could OOM the engines' startup allocation. Offload every tracked
GPU-resident model instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
…ers resident

On TB-scale colocated models (Kimi K2.7: ~1.19TB BF16 masters) the CPU
offload held ~1.34TB/node of pinned/anonymous RAM next to the vLLM engines'
own ~1.3TB level-1 sleep backup — two full CPU copies of the model per
2.9TB node, leaving <100GB of margin. Every bring-up was a coin flip that
ended in NUMA-local OOM kills (CONSTRAINT_MEMORY_POLICY, one socket full
with TBs free on the other) or, with swap present, compress/disk thrash
until Ray's keepalive declared the workers dead.

Two changes to the offload layer:

- Frozen (requires_grad=False, non-adapter) params now offload to
  file-backed mmap tensors, written once per rank on first offload
  (SKYRL_FROZEN_OFFLOAD_DIR, default /data/skyrl/frozen-offload, "0"
  disables and restores pinned RAM). The masters are immutable for the
  whole run, so their pages become clean, evictable page cache instead of
  unreclaimable RAM; re-offloads are free and backloads read hot cache.
- LoRA models skip offloading the fused DDP buffers entirely: Megatron's
  param/grad buffers only hold grad-requiring params, which for LoRA is
  the adapters (a few GB). Keeping them GPU-resident lets the adapter-only
  weight sync export without ever backloading the masters.

Measured on ghost002+ghost013 (2x8xB300): peak-phase MemAvailable went
from 80-380GB (repeated OOM kills across 8 consecutive bring-up attempts)
to ~1.5TB, with bring-up reliably passing.

Co-authored-by: Cursor <cursoragent@cursor.com>
…deleted-tenant live state

Three fixes for the session-churn crash seen on 2026-08-18 (swap_to_adapter
-> CUDA invalid argument at grad copy_, then silent state inheritance on the
retry run):

- _snapshot/_restore skip grad_data copies while the DDP grad buffers are
  offloaded (storage().resize_(0) leaves a stale view; Megatron zero-fills
  on reload anyway, and grads are only offloaded post-step). Snapshot
  records zeros instead of reading freed memory.
- create() no longer adopts the live GPU state when current_id was cleared
  by delete(): a new _live_dirty flag distinguishes "live is pristine"
  (true first create) from "live mirrors a deleted tenant" (must seed from
  pristine, swap_to restores it).
- register_pristine() synchronizes after its non_blocking D2H snapshot so
  create()'s CPU-side _copy_slot can't race the in-flight DMA.

Unit tests fake the DDP buffers via monkeypatched _iter_buffers and cover
the exact production ordering (create new model -> expiry-delete of current
-> swap with grads offloaded).

Co-authored-by: Cursor <cursoragent@cursor.com>
…lora=false)

With merge_lora=false vLLM serves the released base checkpoint and
hot-loads the LoRA adapter, yet the colocated sync still moved terabytes
to deliver gigabytes: backload the frozen masters to GPU, wake the engine
weights, run the broadcast, offload the masters again, wake the KV cache.
On Kimi K2.7 a sync took 13-80 minutes when it survived at all, and the
client saw "stuck on the initial weight sync" timeouts.

save_weights_for_sampler now routes megatron+LoRA+no-merge through an
adapter-only path (_sync_lora_adapters_colocated):

1. collectively export + write the adapter (new worker method
   save_lora_adapters). The export reads only the LoRA adapter tensors,
   which stay GPU-resident through offload (LoRA DDP buffers are exempt),
   so the TB-scale frozen masters remain offloaded: a cold trainer syncs
   in seconds instead of re-faulting TBs of mmap'd frozen weights through
   a page cache the engine build just evicted (that re-fault stalled
   run-start syncs 15-25 minutes and starved client deadlines into an
   abandon/retry loop that rebuilt the engines and repeated the eviction),
2. if a preceding forward/optim phase left masters or optimizer resident,
   offload them so the engines fit,
3. wake the engines only if they were asleep (the backend no longer
   force-sleeps awake engines just to re-wake them mid-sync; the eager
   post-create sleep is also skipped on this path — it was a pure
   sleep->wake round-trip costing 5-6 minutes of ~830GB/node CPU backup),
4. reset the prefix cache and send the adapter load request from the
   engine process once the engines are awake (split out of the worker's
   fused save+load via send_load_request=False).

The classic full-broadcast path is unchanged for FFT / merged-LoRA.
_prepare_for_weight_sync also releases cached CUDA allocator blocks
before engine wake: a preceding forward/optim step leaves tens of GB of
freed-but-cached blocks hoarded in the trainer processes, which pushed
wake_up(tags=["weights"]) over GPU capacity.

persist=True sampler saves on this path tar the just-written PEFT
adapter files (GBs, loadable via vLLM load_lora_adapter) instead of a
multi-TB merged HF export; the classic persist path keeps the HF export
but sleeps the engines first so the trainer backload fits.

SKYRL_LORA_SLEEP_LEVEL=2 opts LoRA runs into discard-and-reload sleeps
for TB-scale models where the level-1 CPU backup (~1.3TB/node) plus the
trainer's own offload buffers exceeds host RAM.

Co-authored-by: Cursor <cursoragent@cursor.com>

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces significant optimizations and robustness fixes for colocated training and inference, particularly for Megatron LoRA configurations. Key changes include file-backed mmap offloading for frozen parameters to prevent host OOMs, an optimized adapter-only weight sync path that avoids backloading massive base weights, and critical fixes in AdapterStore to handle offloaded grad buffers and prevent state leakage from deleted adapters. The review feedback is highly constructive, pointing out a potential cache collision risk in the offload directory, a memory optimization for writing tensors to disk, a potential crash in non-distributed environments within _is_lora_sync_writer_rank, and duplicated path resolution logic in save_sampler_checkpoint.

Comment on lines +253 to +260
def _frozen_offload_file(name: str, tensor) -> str:
import hashlib

rank = torch.distributed.get_rank() if torch.distributed.is_initialized() else 0
key = hashlib.sha1(f"{name}|{tuple(tensor.shape)}|{tensor.dtype}".encode()).hexdigest()[:20]
rank_dir = os.path.join(_FROZEN_OFFLOAD_DIR, f"rank{rank}")
os.makedirs(rank_dir, exist_ok=True)
return os.path.join(rank_dir, f"{key}.bin")

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.

high

The current cache key generation in _frozen_offload_file only hashes the parameter name, shape, and dtype. If multiple training runs or different models (with identical layer names and shapes but different base weights) are executed on the same machine/cluster using the default /data/skyrl/frozen-offload directory, they will silently overwrite or reuse each other's cached files. This will lead to silent, catastrophic model corruption where incorrect weights are loaded.

To prevent this, consider incorporating a unique identifier (such as a hash of the model path, a run ID, or a quick checksum/sample of the actual tensor values) into the cache key or the directory path.

Comment on lines +273 to +276
tmp = f"{path}.tmp{os.getpid()}"
with open(tmp, "wb") as f:
f.write(data.contiguous().view(torch.uint8).flatten().cpu().numpy().tobytes())
os.replace(tmp, path)

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.

medium

Using data.contiguous().view(torch.uint8).flatten().cpu().numpy().tobytes() creates a temporary Python bytes object containing the entire tensor's data in memory. For large model parameters, this can cause a significant memory spike.

Using numpy.ndarray.tofile() writes the array's raw data directly to the file stream, avoiding the creation of a temporary bytes object in the Python heap.

Suggested change
tmp = f"{path}.tmp{os.getpid()}"
with open(tmp, "wb") as f:
f.write(data.contiguous().view(torch.uint8).flatten().cpu().numpy().tobytes())
os.replace(tmp, path)
tmp = f"{path}.tmp{os.getpid()}"
with open(tmp, "wb") as f:
data.contiguous().view(torch.uint8).flatten().cpu().numpy().tofile(f)
os.replace(tmp, path)

Comment on lines +1413 to +1421
cached = getattr(self, "_lora_sync_writer_cache", None)
if cached is None:
import socket

hostnames = [None] * torch.distributed.get_world_size()
torch.distributed.all_gather_object(hostnames, socket.gethostname())
cached = hostnames.index(hostnames[torch.distributed.get_rank()]) == torch.distributed.get_rank()
self._lora_sync_writer_cache = cached
return cached

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.

medium

_is_lora_sync_writer_rank calls torch.distributed.get_world_size() and torch.distributed.all_gather_object() unconditionally. If this code is executed in a non-distributed environment (e.g., during single-GPU testing or mock runs where torch.distributed is not initialized), it will crash.

Add a guard to check if distributed training is initialized, falling back to returning True (since there is only one rank).

        cached = getattr(self, "_lora_sync_writer_cache", None)
        if cached is None:
            if not (torch.distributed.is_available() and torch.distributed.is_initialized()):
                cached = True
            else:
                import socket

                hostnames = [None] * torch.distributed.get_world_size()
                torch.distributed.all_gather_object(hostnames, socket.gethostname())
                cached = hostnames.index(hostnames[torch.distributed.get_rank()]) == torch.distributed.get_rank()
            self._lora_sync_writer_cache = cached
        return cached

Comment on lines +1388 to +1393
base_sync_path = self._cfg.trainer.policy.model.lora.lora_sync_path
adapter_dir = (
os.path.join(base_sync_path, os.path.basename(model_id))
if sync_id is not None
else base_sync_path
)

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.

medium

Currently, save_sampler_checkpoint reconstructs the adapter directory path locally using os.path.basename(model_id). This duplicates the path resolution logic from the worker's _resolve_lora_sync_target and introduces a risk of mismatch if the worker uses a different naming convention (e.g., if model_id contains slashes and is not basename-sanitized on the worker).

Since save_weights_for_sampler already triggers the sync which resolves the correct path, consider modifying save_weights_for_sampler to return the resolved lora_sync_path and use it directly here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 6a0036d. Configure here.

if sync_id is not None
else base_sync_path
)
self._create_tar_from_directory(adapter_dir, output_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Persist tars adapters off the driver

Medium Severity

persist=True on the adapter-only path tars lora_sync_path from the backend process. Writers are the first rank on each GPU node, and that path is allowed to be node-local so engines can hot-load locally. The driver often cannot see those files, so sampler checkpoints fail or archive an empty/stale directory.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6a0036d. Configure here.

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