Support multiple args options by yichuan-w · Pull Request #941 · sgl-project/sglang

yichuan-w · 2024-08-05T16:41:36Z

Thank you for your contribution, we really appreciate it. The following instructions will help improve your pull request and make it easier to receive feedback. If there are any items you don't understand, don't worry. Just submit the pull request and ask the maintainers for help.

Motivation

Support #893 and keep the old ones

Modification

Briefly describe the changes made in this PR.

Checklist

Ensure pre-commit pre-commit run --all-files or other linting tools are used to fix potential lint issues.
Confirm that modifications are covered by complete unit tests. If not, please add more unit tests for correctness.
Modify documentation as needed, such as docstrings or example tutorials.

…store instead, by content key WINDOW-946RF-0828 PROVED THE ROOT RATHER THAN SUGGESTING IT: reason=anchor_no_vote 5 of 5, and "[sgl-project#915 prefetch-gate] no observation" confirming from the other side that the gate was never reached at all. The escape's precondition was locally_eligible = last_host_node.backuped or last_host_node is root_node and `backuped` means "the full KV is ALREADY in THIS rank's host pool" -- local residency demanded as the entry price for an operation whose whole purpose is to obtain what is NOT resident. THE PRECONDITION IS ANTI-CORRELATED WITH THE SITUATION THE ESCAPE EXISTS FOR. Five instances in this family were compensators in the wrong PLACE; this is the first that was in the right place asking the wrong QUESTION. THE REPLACEMENT: presence by CONTENT KEY. `HiCacheController.store_presence_pages` computes the page-hash chain with the SAME helper the real fetch uses (`get_hash_str` -> `batch_exists`, the `_storage_hit_query` path) and asks the backend once. A second spelling of a key chain would be a second installer of the same payload -- the rule #949b was just fixed under, one layer up. WHY THIS IS MORE THAN A BUG FIX. Under sgl-project#706 the store is geometry-neutral: a page carries all attention layers for its tokens and the cut happens at READ time, so the key is a function of CONTENT, not of any rank's layout or residency. A content-key fetch therefore lands in the CURRENT layout and sidesteps the layer-sharded seam problem of sgl-project#941 entirely -- the practical route to cached>0 after a flip, host store instead of device seam carry. ONE GATE BLOCKED TWO COMPENSATORS. sgl-project#939/#943b's re-issue already had its reachability half fixed (sgl-project#946 widened the candidate set to pp_request_locations, scheduler.py:8354) and it still never fired, because the chosen rid is re-issued through this same `_prefetch_kvcache` and died at this same anchor. That is why PREFETCH RE-ISSUED read 0 for seven boots AFTER its candidate set was corrected. One fix serves both. COVERAGE CHECKS BEFORE BUILDING, two of which changed the design: * sgl-project#937/sgl-project#911 generation routing already sits on the insert path (unified_radix_cache.py:3192-3198), so this adds NO second completion path -- the fetch completes through the ordinary route and inherits the refusal. * sgl-project#915's gate is TRAVERSED, not bypassed: only the ANCHOR term changes, so too_short and rate_limited can still speak. That keeps the capacity-asymmetry candidate measurable instead of assumed away. * sgl-project#706 IS landed (six production importers; --phase-flip-canonical-kv-page in the boot), contrary to a stale "when it lands" comment in disk_tier_control.py that would have had this design built on a false absence. * sgl-project#941's kv_reshard seam route is PRICED in the ticket as the alternative and deliberately not built -- it answers a different question. GROUP UNIFORMITY: `prefetch_from_storage` is a collective under `symmetric` (sgl-project#580), so the rank-local presence answer enters the EXISTING vote as `locally_eligible`. Never a new early return, never a new collective -- a rank-local answer that skipped the vote is the sgl-project#580 desync itself. COST, bounded by construction: the whole key chain goes in ONE batch_exists, the verdict is cached on the request per streak increment, and an anchored request short-circuits before any probe -- so the default admission path pays nothing. TESTS: 5 arms RED FIRST, all four failing with the metal reason `declined:anchor_no_vote` verbatim before the fix. Delivery proven by the EFFECT (rid in ongoing_prefetch), never by a return value. Counter-arm: an empty store declines with `declined:store_absent` and never enters the fetch. Can-fail proven by mutation. `anchor_no_vote` survives for tree caches with no controller to ask, because calling that "store_absent" would claim a check never made. Blast radius 44 modules, each alone in a fresh process, identical script both sides vs 3303289: BEFORE 1167 passed / 9 failed, AFTER 1172 / 9. Exactly ONE module differs (946: 42 -> 47). The 9 failures are pre-existing and identical both sides (580: 7, 610: 2). Extraction count probe 9 == 9. ruff (F401,F821,UP037) clean. cache_controller.py is NOT ruff-format-clean at HEAD, so it was NOT formatted wholesale -- the diff is 61 insertions and ZERO deletions; a wholesale run had touched two unrelated asserts and was reverted. The two codespell hits in scheduler.py are pre-existing.

…the refusal never printed VERDICT FIRST: sgl-project#941's named way DOES NOT GO, and the DO-NOT-BUILD stands. This commit builds no carry. It makes the DO-NOT-BUILD's OWN reopening condition measurable, which is the only thing on this path that is both cheap and honest. THE BRIEFED WAY, FALSE ON BOTH LEGS, at the code rather than by argument. Leg A -- "kv_reshard belongs BEFORE the no-return region, where a collective is legal." The pre-cutover hook IS the no-return region. `managers/gdn_flip_mover.py:8` says so verbatim about itself: "Runs as a ``pre_cutover_fn`` of PhaseFlipRuntime, inside the no-return region, BEFORE the weights refill." The loop is `phase_flip_runtime.py:12108`, and it sits AFTER the KV wave loop has already reclaimed and rewritten pool memory -- which is what makes it no-return. There is no earlier legal shelf to move to. Leg B -- "the actuator exists, imported and unused." It exists and it is the WRONG AXIS. `kv_reshard` is the sgl-project#297 DCP row mover: `build_transition(slots, old_vector, new_vector, rank)` (`layers/dcp/reshard_plan.py:216`) derives ROW transitions from the token- ownership vector, and `_PoolView` (`kv_reshard.py:157-203`) wraps THIS rank's own per-layer buffers with `num_layers = len(k_buffers)`. It moves rows within the layers a rank already holds; it never moves a layer between ranks, and its p2p leg hard-raises on size disagreement (`kv_reshard.py:1088`) -- which is precisely what PP peers holding different layer SETS would present. Wiring it into the seam does not reuse an actuator; it would mean building the layer all-to-all from zero, i.e. the exact object `ec1717491f` refused. WHAT IS ALREADY BUILT, so it is not built twice. `seam_layer_carry.py` is wired (`86f6642e5b`, schedule_batch.py:82/2609) and already answers the direction that needs no collective (TP copy -> PP pool, a superset, rank-local slice). The refused direction is the other one: PP copy (4/16 layers) -> TP pool (16), where the missing layers are on a PEER. That is the all-to-all, and it is the refusal. WHY I DID NOT GO AROUND IT. `ec1717491f` is falsifiable and names its own falsifier: it priced the collective against a THIRTEEN-row specimen and wrote "payload is linear in extent while the collective is latency-dominated, so a break-even exists somewhere in the hundreds-to-thousands of tokens. What would settle it is the DISTRIBUTION of `extent` over requests actually retracted at a flip. I do not have it and am not entitled to infer it from any record, in either direction." Overriding that without the distribution would be deciding the same question from the same single specimen, louder. THE DISTRIBUTION WAS NOT HARVESTABLE, AND THAT IS THE DEFECT THIS FIXES. The LAYOUT refusal named the missing LAYERS (through `carry_refusal`) and never the ROWS. The extent-mismatch refusal above it is the only other emitter of `covered`, and by construction it never fires for a request that reaches the layout branch -- it returned already. So the gate deciding whether the carry may EVER be built could not be read off a boot log at all. 57 refusals were 57 discarded samples of exactly the number needed. `sgl-project#941 extent=` now greps them. AN INSTRUMENT, NOT A GATE. Nothing reads the value; no branch changes; the refusal is byte-identical in behaviour. It supplies the middle term of (missing_layers x extent x row_bytes) -- `carry_refusal` already carries the layer count -- so the next window can price it against the sgl-project#656 PHB numbers (43.9 KiB/crossing, 166 us enqueue, 1777-9201 us receive) instead of re-arguing it. The DO-NOT-BUILD stands until that product is measured. SEPARATELY, AND IT IS THE LARGER FINDING: the route to cached>0 after a flip is ALREADY ON THE LINE AND HAS NEVER BEEN MEASURED. The tree states it itself at `cache_controller.py:2320-2325` -- a content-key fetch "lands in the CURRENT layout and sidesteps the layer-sharded seam problem of sgl-project#941". sgl-project#950 (`dc4895e1dc`) and #869b (`44b0b33c07`) are both ancestors of `merge/flip-window-0828`. Reachability from the defect path is wired, checked as a call chain and not assumed: `readmit_seam_residents` (scheduler.py:5195) -> `_add_request_to_queue(is_retracted=True)` (:5111) -> `_prefetch_kvcache` (:5123, disaggregation_mode NULL) -> the sgl-project#950 content-key escape (:5011-5040) -> `store_presence_pages` (cache_controller.py:2306) and `scheduler.py:5173` calls that hop load-bearing in its own words: " `_prefetch_kvcache`, which is what makes the promised read-through actually hit." THE 57/57 MEASUREMENT PREDATES ITS OWN FIX. It was taken 2026-08-27 ~13:0x (REGISTER_OPEN_876.txt:6670); sgl-project#950 landed 2026-08-28. window-950-boot then measured nothing -- the scheduler died before doing work, every counter 0 for that reason and not because sgl-project#950 failed (TICKET_944, PASS/FAIL table, six rows NOT MEASURED). So the symptom driving sgl-project#941 has not been re-observed on a tree that contains the thing aimed at it. EVIDENCE TIER, explicitly. DESK-PROVEN only. No boot, no card, no serving. The two falsehoods in the briefed way are proven at file:line and are the strongest claims here. Nothing is "fixed": the emitter is inert, and whether sgl-project#950 turns 57/57 cached=0 into cached>0 is a metal question that is still open. CHECKS RUN (hermetic, CUDA_VISIBLE_DEVICES="", /spinning/htsglang-gpu/.venv): * AST parse of schedule_batch.py: OK. * AST-located LAYOUT emitter: exactly ONE in the file; placeholders 9 == args 9; rendered with stand-in values and asserted to contain `sgl-project#941 extent=4096`. * CAN-FAIL, both arms: the pre-fix format string has 8 placeholders and reds the `sgl-project#941 extent=` assertion; an arity mutant (text added, arg forgotten) raises `TypeError: not enough arguments for format string` -- i.e. the check catches the one way this edit could have broken a log call at runtime. * CPG chain above verified by AST call-graph, not by grep. No suite: speed mode, suites run once at the end. (cherry picked from commit 1d3b95e)

…se that could not fail is gone BOOT 17 AND BOOT 18 DIED OF THIS, at the first genuine idle, rank-uniform: `ValueError: pool memory leak detected! [full]` from `on_idle`. The admission wedge that came with it was a symptom -- 99 ADMITs still happened in the same window -- and the timeout that ended the load driver did not cause the death, it EXPOSED it by producing the first idle the boot ever had. THE CHAIN, MEASURED, five boots and no counter-instance (loadback-genuine / orphan-events / max-unaccounted): 1043b 0/0/0, 1043c 0/0/0, 1046cut 3/18/54626, 1048fix 69/21/43803, 1049n9 30/6/21608. Zero load-backs -> zero orphans -> zero ratchet. The orphan sizes are LITERALLY the load-back prefix lengths (5834, 4618, 350), rank-uniform on PP0/PP1/PP2, and the last census `unaccounted=` before death is identical to the size of the killer's `leaked_full_pages` set (21608 = 21608, contiguous 1..21608). An identity, not a plausibility. So this is not a regression of the load-back. It is sgl-project#938 -- registered open since 2026-08-27, Stage A (instrument) shipped as 226d34e, Stage B never built -- made FATAL because the load-back finally works: whole 4618-token prefixes are locked at the drop where the register's pre-registered fork expected ~283 rows. That fork is hereby answered from metal, 16x larger. WHAT CHANGES * `UnifiedRadixCache.reclaim_rows_for_drop` returns every row the tree still holds, locked ones included, and `drop_prefix_tree_returning_rows` calls it before `reset()`. `evict` refuses locked nodes and `_reset_full` zeroes the protected book without freeing a row, so those rows were leaving with nobody owning them, one drop's worth per flip cycle. * THE sgl-project#938 PREMISE IS NOW CHECKED, NOT ASSUMED. sgl-project#938 refused to free because a lock here is supposedly an in-flight write-through, and freeing under a live reader is a use-after-free in the sgl-project#913 family. Correct -- so the reclaim runs ONLY when `ongoing_write_through` is empty. On boot_855_1049n9 all 13 `sgl-project#792 post-retract writeback fence` lines report `outstanding=0`, including the two drops that orphaned 5834 and 4618 rows: the in-flight explanation held on no drop of that boot. When a copy IS outstanding the reclaim refuses and prints the count -- today's behaviour, kept. * NEVER FREES BLIND. Candidates are differenced against the allocator's own enumerated free set (`read_free_rows`, the union the idle invariant and the sgl-project#822 census already share). A non-enumerable reading refuses outright: a watermark allocator can say HOW MANY rows are free and not WHICH, and freeing against a count would be inventing membership. Double-freeing a row is silent corruption -- the one outcome worse than the leak. * The free is addressed through `FullComponent._free_full`, which resolves the allocator per access (sgl-project#941): a captured binding lands the rows on the other phase's free list as duplicates, the same symptom with a different root. * THE DEFERRAL IS INSTRUMENTED UNCONDITIONALLY. `sgl-project#1050 CUTOVER ROW RECLAIM` prints reclaimed/full_rows/mamba_slots/full_held/mamba_held/already_free and the refusal reason on every drop, including the all-zero reading. A clearer that can silently never run is how the same ratchet comes back one level up in a new pocket. * sgl-project#919's EXCUSE COULD NOT FAIL, AND IS GONE. The audit reported "pp_stack_allocator owns ids [1, 578995) and covers every sampled row" as the explanation for 43803 unowned rows, against a census id space of 578994 -- a containment test true for every sample that could ever be drawn. It excused a real, monotone, ultimately fatal loss. `OwnerCandidate` now carries `discriminating`, set false when the range is not narrower than the census's own, and such a candidate can no longer close the question: the new `SECOND-POOL-SPANS-EVERYTHING` verdict says the block is NOT excused. CHECKS (matched to this edit's failure classes, not a battery) * devtools/check_1050_cutover_reclaim.py, 17 cases, all pass: the double-free guard (an already-free row is not re-freed, and the count is reported), the outstanding-write-through refusal with its number, the non-enumerable refusal, root never harvested, and the no-mamba lane. py_compile is structurally blind to all of these -- `component_data` is a LIST indexed by the ComponentType enum, not a dict, and a `.get` there would have raised on every drop, on metal, mid-flip. * devtools/unaccounted_watchman.py --selftest, 4 cases, all pass, INCLUDING a replay of the real boot-18 log. That case earned its place: the first version keyed on the rank alone, passed its synthetic staircase, and read SILENT on the very boot it was written for, because the census emits four readings per cycle and the PP-layout zeros reset the streak. Keyed on the census family it fires. A watchman validated only against its own fixture is not validated. * ruff clean, py_compile clean on all three modules. NOT YET BOOTED -- desk-proven only.

…ree actually reaches Self-caught before the boot landed, and it is the same mismatch class as sgl-project#941 one level up: not WHICH object is held, but which object is ASKED. `_free_full` routes to `full_attn_allocator` when SWA is in the tree (full_component.py). The guard read its free set off the TOP allocator unconditionally, so under SWA it would have checked membership in one pool and freed into another -- the guard passing while performing exactly the double free it exists to prevent. INERT ON THE BOOTED CONFIG (hybrid-SSM, no SWA), so boot 19 @ e051499 measures the same behaviour either way; this is correctness for the SWA lane, not a change to what the boot proved. check_1050_cutover_reclaim.py 17/17, ruff + py_compile clean.

…mamba slots blind Twenty lines above the loop this fixes, the same function reads the allocator's enumerated free set and frees only rows that are NOT already on it, under a comment that states the rule in full: "NEVER FREES BLIND. A row that is already on the free list must not be freed twice: that is silent corruption, the one outcome worse than the leak." Every word applies to a mamba slot. None of it was implemented on the mamba side. MEASURED, boot 25 (boot_855_1054diag_0840f82601_0831_143640.log): nine MambaSlotDoubleFree events, all three ranks, and the #1033b provenance put BOTH releasers inside this one function -- the eviction pass at :1924 frees a node's `cd.value` and nulls it, then this loop walks the tree again and returns the same slot a second time. Neither releaser was at fault. The asymmetry was. THE AUTHORITY IS THE ALLOCATOR'S OWN LEDGER. `slot_used` is the sgl-project#924 bool ledger and answers "is this slot already free?" in O(1) -- the same question `read_free_rows` answers for the KV half, with a cheaper instrument and the same refusal to guess. AND IT ASKS THE ALLOCATOR THE FREE ACTUALLY REACHES (sgl-project#941 class, the lesson the KV half spells out one screen up): `_free_mamba_value` routes to `int8_ckpt_pool` when one exists, so differencing against the SLOT allocator's ledger would check membership in one pool while freeing into another -- the guard passing while performing the exact double free it exists to prevent. With a checkpoint pool in play there is no enumerable ledger here, so this REFUSES and says so, rather than freeing blind. Same for an allocator that exposes no ledger at all. NO SILENT `continue` ON THIS PATH ANY MORE. The version replaced here swallowed every per-slot failure without a word. That is the C1 swallow instance in miniature and it is what let boot 25's nine raises die unheard while the ledger corrupted underneath: the boot survived, which is why nobody saw it. Failures are now counted (`mamba_failed`), the first one's type AND message land in `reason`, and a loud ERROR names them. The flip still proceeds -- a reclaim may not abort a seam -- but it can no longer proceed QUIETLY. Skipped already-free slots are counted separately (`mamba_already_free`) and warned, because a non-zero there is not noise: it means two references to one slot survived into the same drop. ROOT BEFORE EFFECT * CLASS: one half of a paired guard implemented, the other left blind -- the same shape as sgl-project#1051 (ownership transfer honoured on one side only) and as sgl-project#929 before it. Third instance in this family on this path. * SIBLING SWEEP: the KV half already differences and already refuses on a non-enumerable free set; the host/mamba host pool is freed through `_mamba_pool_host` under the component's own eviction, not here. * FUTURE CHECK: `devtools/check_1055_mamba_reclaim_differencing.py`, hermetic, no CUDA. It calls the REAL `reclaim_rows_for_drop` UNBOUND against a stand-in `self`, so the shipped body is what runs: an already-free slot is not returned twice, owned slots still are, the skip is counted, the int8 and no-ledger paths refuse with a stated reason, and the swallow is gone. 7/7. It caught one defect in ITSELF while being written -- a source-slice window too short to reach the code it asserted, i.e. a truncated read scored as absence, the same family as the `| head` trap; the window is now bounded by the function's own return. EVIDENCE TIER: DESK-PROVEN. The metal proof is boot 26's MambaSlotDoubleFree column, which is unpassable without this.

sipport multiple args

c8e4919

yichuan-w requested review from Ying1123 and merrymercy August 5, 2024 16:41

zhyncs reviewed Aug 5, 2024

View reviewed changes

Comment thread python/sglang/srt/server_args.py Outdated

fix

9ece4a3

zhyncs approved these changes Aug 5, 2024

View reviewed changes

zhyncs merged commit ffb1574 into sgl-project:main Aug 5, 2024

yichuan-w mentioned this pull request Aug 5, 2024

[Feature] Add alternative names for pp-size, tp-size, and dp-size #893

Closed

timethink pushed a commit to timethink/sglang that referenced this pull request Mar 9, 2025

Support multiple args options (sgl-project#941)

14fcd04

Navigation Menu

Uh oh!

Support multiple args options - #941

Support multiple args options#941
zhyncs merged 2 commits into
sgl-project:mainfrom
yichuan-w:args_rename

yichuan-w commented Aug 5, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

yichuan-w commented Aug 5, 2024

Motivation

Modification

Checklist

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants