Skip to content

[sglang-miles] Fix LoRA usage-counter lifecycle across cleanup paths - #33300

Open
Shi-Dong wants to merge 44 commits into
sgl-project:sglang-milesfrom
Shi-Dong:shi/lora-release-on-abort
Open

Shi-Dong wants to merge 44 commits into
sgl-project:sglang-milesfrom
Shi-Dong:shi/lora-release-on-abort

Conversation

@Shi-Dong

@Shi-Dong Shi-Dong commented Aug 3, 2026

Copy link
Copy Markdown

Problem

LoRARegistry.acquire() increments an in-flight usage counter before request
validation/dispatch, while /unload_lora_adapter waits for that counter to
return to exactly zero. Request cleanup paths that miss a release leave unload
blocked forever; duplicate releases can drive the counter below zero and have
the same effect.

The original version of this PR fixed the obvious abort and failed-dispatch
leaks by coupling rid_to_state.pop() with release(). That is not sufficient
for native parallel sampling (n > 1), where parent states, regenerated child
states, and a prefix-cache warm-up request all observe the same acquired LoRA
references.

Fix

This revision incorporates the lifecycle model from #31808 into the
sglang-miles branch and extends it for the request shapes used here:

  • Add a per-ReqState exactly-once release guard. Normal completion, abort
    echo, scheduler error, and failed dispatch all route through the same helper.
  • Keep strong references to asynchronous release tasks until they complete.
  • Treat the acquired lora_id, rather than lora_path, as the ownership
    signal. This avoids release(None) when acquire never succeeded and remains
    compatible with [lora-related] feat(sglang-miles): Support upsert when loading adapters from tensors/distributed #30913, where an upsert deliberately reuses the stable
    adapter ID.
  • For parallel sampling, make the prefix-cache warm-up state a non-owner and
    transfer the acquired references from bookkeeping parents to exactly
    batch_size * n regenerated child states.
  • Support an explicit parent rid list: one parent state temporarily owns the
    n references for its prompt until fan-out, so validation/tokenization
    failure before fan-out still returns every acquired reference.
  • Close the abort-vs-acquire race. If an abort removes a tokenizer-held state
    while registry acquisition is awaiting, the newly acquired orphaned ID is
    returned immediately.

The resulting invariant is:

Every successful LoRA acquire has exactly one owner and exactly one release,
across success, abort, validation failure, and native multi-LoRA parallel
sampling.

Relationship to earlier LoRA PRs

Tests

PYTHONPATH=python python3 -m pytest -q \
  test/registered/unit/managers/test_tokenizer_manager_rid_cleanup.py \
  test/registered/unit/managers/test_abort_request_prefix.py::TestAbortTokenizerHeldRequests \
  test/registered/unit/managers/test_io_struct.py

54 passed, 14 warnings in 11.74s

The lifecycle suite includes single and batch completion, abort/finish races,
acquire failure, abort racing acquire, n=4 prefix warm-up accounting, two
simultaneous LoRA adapters with explicit parent rids, and pre-fan-out failure.

Additional checks:

ruff check --ignore E402 <changed files>  # passed
python3 -m py_compile <changed files>     # passed
git diff --check                         # passed

CI States

Latest PR Test (Base): ❌ Run #31412296286
Latest PR Test (Extra): ❌ Run #31412295918
Latest PR Test (AMD ROCm 10): ➖ No AMD PR run found for this commit.

ishandhanani and others added 30 commits July 22, 2026 15:18
…ect#31076) (sgl-project#32074)

Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Co-authored-by: Connor Carpenter <connorc@nvidia.com>
…yer mtp draft extend cuda graph (sgl-project#32254) (sgl-project#32260)

Co-authored-by: Qiaolin Yu <liin1211@outlook.com>
…isoning spec verify capture (trtllm_mla + tc_piecewise) (sgl-project#32288) (sgl-project#32346)

Co-authored-by: Qiaolin Yu <liin1211@outlook.com>
…ense) (sgl-project#18639)

Squashes the qwen_dense on-policy follow-up into the original FSDP2 commit:
the follow-up rewrote the norm_kwargs / rl_on_policy_target call sites this
commit introduced, so they cannot be applied independently.

Rebased onto v0.5.16: rl_on_policy_target predicates now go through the
sglang.srt.true_on_policy helpers, and every legacy get_global_server_args()
call site uses runtime_context.get_server_args() so v0.5.16's
test_legacy_global_ratchet baseline still holds.

Co-authored-by: maocheng23 <35615230+maocheng23@users.noreply.github.com>
…h/RL fixes (sgl-project#25141, sgl-project#29874, sgl-project#31251)

Squashes the two follow-up PRs into the MoE-LoRA base commit: both rewrite the
mem_pool sharding helpers this commit introduces and cannot be applied
independently.

Rebased onto v0.5.16:
  - the shard probes keep v0.5.16's shared-MoE-over-full-TP carve-out at EP=1
    (`is_shared_moe_module`) and only fall through to the probed shard for
    non-MoE modules;
  - sgl-project#29874's `free_lora` is dropped -- v0.5.16 already releases the slot on
    unload through `LoRAMemoryPool.remove_lora()`, which additionally zeroes the
    buffers for graph-captured replay, so `lora_manager.unload` needs no second
    call;
  - sgl-project#31831's expected_checksums check is likewise already upstream in
    `tp_worker.load_lora_adapter_from_tensors`.

Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai>
…psert (sgl-project#27268, sgl-project#31759, sgl-project#30913)

Squashes the two follow-ups into the API commit: sgl-project#31759 fixes this commit's
HTTP handler (the LoRAUpdateOutput was returned unserialized) and sgl-project#30913
rewrites its registry-registration block to support in-place upsert, so
neither applies on its own.

Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai>
Co-authored-by: Mathew Han <49226490+mathewjhan@users.noreply.github.com>
…ct#21278, sgl-project#22663)

Includes making Cohere2MoeConfig a dataclass so parameter mapping can
introspect config fields for P2P weight update.

Rebased onto v0.5.16, which extracted the remote-instance transfer engine out
of ModelRunner into RemoteInstanceWeightTransporter: the per-rank
RankParallelismConfig is now built in `init_engine()` and published from
`maybe_register_and_publish_weight_info()`, instead of the ModelRunner methods
this commit originally added. The hoisted deepseek expert_params_mapping also
picks up v0.5.16's broader `is_wint4afp8_or_wint4a16_config` predicate rather
than the old `quant_config.get_name() == "w4afp8"` check.

Co-authored-by: JensenFire <elevenji@outlook.com>
… for spec draft worker(s) (sgl-project#27749, sgl-project#28575, sgl-project#18565, sgl-project#22663, sgl-project#28001, sgl-project#29675, sgl-project#27750)

Squash of the spec-draft distributed-update work and the weight-processing
session work: the latter rewrites iter_draft_runners()/get_model_runners() the
former introduces, so they cannot be applied independently.

  - Draft runners never join the update group, so the target receives the
    broadcast once and the weights are loaded into every selected runner.
  - Begin/EndWeightUpdate session chain: engine / http_server /
    tokenizer_control_mixin (pause-aware locking) -> scheduler ->
    SchedulerWeightUpdaterManager, with {target,draft,all} runner selectors.
  - loader.py: public post_load_weights plus restore_weight/postprocess_weight
    via _apply_quant_method_hook (skips LoRA wrappers).
  - weight checker: skip_tensor_list + role-prefixed overall_checksum over
    get_model_runners(selector), replacing _get_draft_model_runner.

Reimplemented onto v0.5.16, which had already extracted weight updating into
ModelRunner.weight_updater (WeightUpdater): the receive/load split lands there
as receive_weights_from_distributed() + load_weights() rather than on
ModelRunner, and the scheduler fan-out drives runner.weight_updater.*. The
worker-level update_weights_from_{distributed,tensor} entry points on
TpModelWorker / EAGLEWorkerV2 / NGRAMWorker are dropped: the scheduler now owns
the fan-out, and leaving them would be a second path that updates one runner
only. iter_runners() replaces the ad-hoc draft-runner discovery on every spec v2
worker (incl. DFlashWorkerV2). The unit test moves to test/registered/rl/, since
v0.5.16 retired test/srt/ from CI collection.

Co-authored-by: Yueming Yuan <yym022502@gmail.com>
Co-authored-by: JD-ETH <jaedon.guo@gmail.com>
Co-authored-by: maocheng23 <35615230+maocheng23@users.noreply.github.com>
…ht updates (sgl-project#30421)

The per-rank attn_sink slice is built once and cached; an RL weight update
rewrites attn_sink in place, so decode keeps serving the pre-update sink.
Refresh the cache from post_load_weights instead.

Extended past the original PR for v0.5.16, which added deepseek_v4_dspark.py
with the same lazily-cached slice: refresh_attn_sink_cache moves from MQALayer
up to MqaAttentionBase (with an overridable _attn_sink_pad_width, since DSpark
pads q to _PAD_NUM_HEADS rather than padded_num_heads), and
DeepseekV4ForCausalLMDSpark gains the matching post_load_weights hook. Without
this, DSpark + RL would keep the same stale-sink bug this commit fixes for the
non-DSpark path.
… into a host-local checkpoint (sgl-project#30366, sgl-project#28524)

Folds in the two fixes sgl-project#28524 made to this endpoint -- Annotated[..., Body()]
on the handler and keyword construction of PullWeightsReqOutput, which is a
kw_only msgspec Struct and would raise on the positional form. The rest of
sgl-project#28524 (dumper / dump-comparator) is already upstream in v0.5.16 and is dropped.
…h the merged per-role payload

`_merge_checksum_payloads` folds each runner's payload into one, tagging every
entry with its role, so `parallelism_info` is a LIST of per-role entries. That
shape was never validated on v0.5.15 -- v0.5.16 added the msgspec wire structs
plus `msgspec.convert(p, ChecksumInfo)` in `check_weights`, and the wire struct
still declared a single `ParallelismInfo` with no `role`, so every
check_weights('checksum') call would have raised on conversion.

This is the shape miles already consumes: `checksum_utils._gpu_rank` iterates
`parallelism_info` and reads `role_info["rank"]` per role, and its tests cover
the target+draft case sharing one GPU rank.
…t to v0.5.16's parallel state

Two follow-ups to [14/24]'s P2P weight-update support, both caused by v0.5.16
retiring the module-level dp-attention accessors. Found by CI: every GPU stage
that initialises the remote-instance transfer engine died at startup with

    remote_instance_weight_transporter.py:56 init_engine
      -> RankParallelismConfig.from_parallel_state
    ImportError: cannot import name 'get_attention_cp_rank'
                 from 'sglang.srt.layers.dp_attention'

1. `from_parallel_state` imported get_attention_{tp,dp,cp}_{rank,size} from
   dp_attention. v0.5.16 removed all six; the attention-side ranks and sizes now
   live on the runtime ParallelState, so read them from `get_parallel()`.

2. `ParallelismContext` set `dp_attention._ENABLE_DP_ATTENTION_FLAG`, which
   v0.5.16 replaced with `get_flags().dp.enabled`. That assignment would not have
   raised -- it would have created a stale attribute nobody reads, leaving
   `is_dp_attention_enabled()` reporting the ambient value instead of the
   context's and silently building a replica with the wrong sharding. The flag is
   now saved, set and restored on the runtime flags instead, and dropped from
   _DA_GLOBALS.
Three stale references left by the rebase. The cherry-picks applied cleanly
because the surrounding lines matched, so nothing surfaced until the code ran.

1. model_runner.forward: `decode_cuda_graph_runner.num_tokens_per_bs` ->
   `.captured_req_width`. v0.5.16 renamed the per-request capture width (and
   `ModelRunner.decode_num_tokens_per_bs` -> `decode_num_tokens_per_req`), so
   every decode with a captured graph raised AttributeError.
   From [2/25], R3 DeepEP/MTP support.

2. lora/backend/base_backend: `dp_attention.get_attention_tp_size()` ->
   `get_parallel().attn_tp_size`. Same removal as [24/25]; this one is a lazy
   import so it only fired on the MoE-LoRA DP-attention path.

3. lora/backend/base_backend: `layers.utils.cp_utils` ->
   `layers.cp.padding` for get_cp_padding_align_size. The module moved in
   v0.5.16; every other caller in the tree already uses the new path.

Found 1 and 2 in CI; found 3 by then resolving every sglang symbol imported by
lines this branch adds against the v0.5.16 tree, which is how it should have been
checked in the first place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blackwell SM100/SM120 deepseek dependencies Pull requests that update a dependency file jit-kernel lora model-gateway Multi-modal multi-modal language model quant LLM Quantization

Projects

None yet

Development

Successfully merging this pull request may close these issues.