[fix][megatron] Make multi-LoRA adapter swaps safe against colocation offload - #2000
Open
erictang000 wants to merge 1 commit into
Open
[fix][megatron] Make multi-LoRA adapter swaps safe against colocation offload#2000erictang000 wants to merge 1 commit into
erictang000 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds support for multi-LoRA training and adapter swapping on the colocated Megatron path. It introduces gradient 'parking' and 'unparking' mechanisms to preserve accumulated gradients across CPU offloads, ensures proper GPU residency of models and optimizers during swaps and optimizer steps, and adds a comprehensive test suite to verify these colocated behaviors. I have no feedback to provide as there are no review comments.
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
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.
[fix][megatron] Make multi-LoRA adapter swaps safe against colocation offload
Problem
Running a multi-tenant Tinker script that both trains and samples crashes on the Megatron backend:
Multi-tenant scripts that only train are fine; the failure needs a
sample.Root cause
Under
colocate_all(the SkyRL-Train default, and what the Tinker API assumes when the operator passes no override), the dispatch offloads policy state between requests. Megatron's grad-buffer offload does not move grads to CPU — it frees them withstorage().resize_(0)and zero-fills on reload. The tensor object keeps its full shape, so nothing looks wrong until a copy dereferences the zero-sized storage:Train-only scripts survive because
forward_backwardbackloads everything (need_optimizer=True). Two more instances of the same shape were sitting next to it:forward/forward_from_stagedbackload only the model, so a swap during a plain forward runs against the same freed grad buffers.optim_stepdid no residency check at all, so a swap there could hit freed param storage once another tenant's sample had offloaded the model.Underneath the crash was a silent correctness bug: because the offload drops grads rather than saving them, grads a tenant accumulated in a
forward_backwardwhoseoptim_stephasn't arrived yet are destroyed when another tenant's request lands in the gap — which is exactly what Tinker's request-granularity interleaving produces. That tenant'soptim_stepthen applies an all-zero gradient and silently does nothing.Fix
adapter_store.py_is_resident()gates every DDP-buffer read/write. Grad copies are skipped while the buffers are offloaded; params must be resident, and a swap without them now raises a clearRuntimeErrorinstead of a CUDA error.park_grads()/unpark_grads(): the live adapter's grads are saved into its own CPU slot immediately before an offload and re-materialised on backload. Unparking restores whichever adapter is live at that moment, which need not be the one parked — a swap inside the offload window only moves CPU slots around, and this is where the result lands back on the GPU.The optimizer needs no special handling: its offload genuinely moves tensors to CPU, so the store's copies degrade to CPU→CPU and the values ride back to GPU on the next backload.
megatron_worker.py—MegatronPolicyWorkerBasebracketsoffload_to_cpu/backload_to_gpuwith park/unpark.worker_dispatch.py—ensure_active_adapterguarantees model residency before swapping (coversset_lr, which only asks for the optimizer);optim_stepensures residency, which is not redundant withforward_backward's under multi-tenancy.Test plan
New
tests/tinker/skyrl_train/test_multi_lora_megatron_colocated.py— sibling oftest_multi_lora_megatron.pywithcolocate_all=True. The existing module pins it toFalse, which is why none of this was covered. CI already globstests/tinker/skyrl_train/, so the module is picked up with no workflow change.maintest_sample_non_live_adapter_colocatedtest_pending_grads_survive_other_tenant_sampleoptim_stepis a no-op (Δ=0.0) vs the control's −4.6e-02test_two_adapters_train_and_sample_colocatedThe grad test measures A against an undisturbed control adapter rather than asserting
post < pre: a bare inequality passes even with grads dropped, because Adam still applies weight decay to a zero gradient and drifts the loss by ~1e-6. Sensitivity was confirmed by disabling the parking hook and re-running.Also run:
tests/tinker/skyrl_train/test_multi_lora_megatron.py— 6 existing non-colocated tests, no regression (9/9 with the new module in one session).tests/backends/skyrl_train/gpu/gpu_ci/test_worker_dispatch_offload.py -k "training_switch or gpu_state_tracking or only_one_model"— 3 passed, covering the FFT colocated path touched inoptim_step.Not run:
tests/backends/skyrl_train/distributed/test_worker_dispatch.pyhangs in Ray worker registration on the dev host — identically on a clean tree with these changes stashed, so pre-existing, but it does leave the dispatch unit tests unverified locally.