Minor fix in compiler & format by ZackZeng999 · Pull Request #545 · sgl-project/sglang

ZackZeng999 · 2024-06-14T01:57:38Z

#532 I fixed this issue and some problems in test/srt/model

Qubitium · 2024-06-14T03:45:22Z

@ZackZeng999 Please modify PR title so it better reflect what the code does.

ZackZeng999 · 2024-06-14T05:02:26Z

@ZackZeng999 Please modify PR title so it better reflect what the code does.

I wonder if this title is acceptable.

Ying1123

Please run scripts/format.sh to format the code.

* parallel model model

MI35x tests hardcoded HF_HOME=/data2/models/huggingface which is ephemeral pod-local storage in Kubernetes. When pods recycle, the model cache is lost, causing tests to re-download ~400GB models from HuggingFace and timeout. The CI container already sets HF_HOME=/sgl-data/hf-cache (backed by a persistent volume) via amd_ci_start_container.sh. MI30x tests correctly rely on this, but MI35x tests overrode it with /data2. Remove all /data2 references so MI35x tests use the same persistent cache path as MI30x. This fixes nightly timeout failures caused by pod recycling (e.g. Nightly Test AMD #545).

…ed: broken control arm, no render bug Outcome (c) of the discrimination matrix: probe-arm mix-up. probe_preserve_thinking.py:89-90 builds its control as 'if preserve: body[...] = {...}', so the false arm sends NO kwarg. That was a valid control only before this boot set a server default. With --chat-template-default-kwargs '{"preserve_thinking": true}' omission now INHERITS true, so both arms ran preserved and the 77.2 vs 6.4 spread is not attributable to the kwarg. Both arms rendered within one token of each other (993 vs 992), so the spread was cache state read through global counters on a shared server. First-divergence matrix, both fronts against ONE raw /generate stream (offline tokenisation, so the parser's lossy reasoning/content split does not contaminate it): OpenAI reasoning_content pv=true 345/345 (100.0%) byte-exact Anthropic wrapped in content pv=true 345/345 (100.0%) byte-exact OpenAI pv=false 29/345 ( 8.4%) diverges Anthropic pv=false 29/345 ( 8.4%) diverges Hypothesis (b), whitespace/re-render mismatch, is REFUTED: at true the re-render reproduces the whole assistant turn including the think block and its close, token for token. The paths agree because the Anthropic front prepends wrap_reasoning_history() as a plain text block rather than as reasoning_content, and both shapes render the same once kept. Consequence for the default-flip decision: the template STRIPS the think block unless preserve_thinking is set, so flipping the default off would CAUSE the prefix collapse it was suspected of -- 100% down to 8.4%, on both fronts, with the Anthropic path unable to compensate per-request. Recommendation: keep the default on. This matches the battery's own 62.9 vs 66.9 percent. Two defects recorded: the Anthropic front ignores chat_template_kwargs entirely (82 tokens for both true and false, against 48 vs 82 on the OpenAI front), so only the server default reaches it; and the probe is invalidated by the default it tests, corrected version added. sgl-project#545: POST /hicache/storage-backend/resize returns HTTP 400 admin_api_key_missing -- the boot has no --admin-api-key, so the live gap stays open pending that flag. The max_size cap was however validated under real battery load: the tier grew about 20 GB/min and then flattened (4636 MB per 20 s down to 46 MB per 20 s), so 100Gi engages and holds instead of filling the filesystem.

…nd fold the B1 survey ## sgl-project#545 -- attach, resize and detach the HiCache disk tier at runtime mem_cache/disk_tier_control.py, flag-gated, 14 hermetic tests. Built through the existing HiCacheStorage/HiCacheFile abstraction: the controller owns the BUDGET AND LIFECYCLE, the bytes stay with the backend. No second directory layout is introduced, so the sgl-project#706 canonical-page store (not on this branch yet) shares the same backend rather than fighting it. Three rules, each because its violation is SILENT: 1. SHRINK EVICTS DOWN TO THE BOUND; IT NEVER TRUNCATES. A truncated page is not a miss -- it reads back short and wrong, which no cache-hit metric shows. Test: 90 MB across nine pages shrunk to 50 MB evicts four WHOLE pages; every surviving page keeps its full size. 2. DETACH IS REFUSED WHILE ANY PAGE IS ONLY-COPY HERE, naming the count and an example key. Detaching would destroy them and the later miss would have no visible cause. force=True exists for an operator who has decided the loss is acceptable, and says so loudly in the log. 3. A SHRINK THE EVICTOR CANNOT HONOUR IS REFUSED, NOT PARTIALLY APPLIED. Accepting a bound the tier is not under would make the reported capacity a lie, and the only way to reach it would be truncation. Capacity is left unchanged on refusal. Also: a second attach is refused (two directories under one controller would give the evictor two budgets and one bound), operations on an unattached tier refuse, and the flag gates every mutation because attaching or resizing a live L3 changes what the evictor is allowed to delete. ## sgl-project#616 -- B1 survey folded into DESIGN_704, revising three things I verified the survey's two load-bearing claims at file:line before folding. 1. "A DCP-TYPED GROUP" WAS IMPRECISE. There are no typed group classes: every group is a bare GroupCoordinator (parallel_state.py:564) via init_model_parallel_group (:2550). "Typed" here means exactly a module global, a named getter, and a routing flag. B1 subclasses and registers nothing -- a smaller and more precise change than the earlier draft implied. 2. THE PRECEDENT ALREADY EXISTS IN-TREE; REUSE IT. initialize_phase_flip_secondary_groups (:3422-3517) ALREADY creates a DCP group in the same world as a PP group. Verified at :3470-3482: it plans flip_tp -> flip_dcp -> flip_pp in fixed order, then :3484-3497 verifies a world-wide manifest by all_gather with an equality check BEFORE creating anything. My earlier draft proposed reconstructing PP rank arithmetic inline; that is unnecessary -- the precedent computes the same range(idx, world_size, num_pp_groups) layout and already sequences it. 3. THE ORDERING WINDOW IS NARROWER THAN FEARED. _DCP at :3152, _PP at :3365, and only the kvso-spill block (:3185-3188) reads _DCP between them. Routing precedence must also be chosen deliberately: get_dcp_group resolves flip route first (:2709), then kvso-spill (:2718), then primary (:2720). Inheriting whichever branch matches first is how a decoupled read would silently land on the flip's secondary group during a phase change. ## NAMED PREREQUISITE, before B1 rather than inside it pp>1 AND dcp>1 is not refused anywhere in the group path, and ParallelContext.dcp_enabled (runtime_context.py:331-337) returns True whenever a DCP group exists with dcp_size>1. Creating B1's group therefore makes dcp_enabled TRUE ON PP PREFILL RANKS, contradicting the assumption dcp_group_guard.py:38-42 states in prose. The failure shape is what makes this worth naming: the guard itself would still PASS, because both sides would read the same new size. What breaks is every OTHER consumer of the documented invariant. The combination is unreachable today, so the contradiction is latent -- and B1 is exactly the change that makes it live. Decide it before building: either accept pp>1 and dcp>1 deliberately and audit the dcp_enabled consumers, or refuse the combination and give B1 its own flag instead of overloading dcp_size. 35 mem_cache tests green, hermetic, ruff + codespell clean.

…; retract my "resize is untested" claim NOTHING BUILT, because it is already built. attach (PUT), detach (DELETE), status (GET), resize (POST /resize) and clear all exist on /hicache/storage-backend (http_server.py:1395-1510), each behind @auth_level(AuthLevel.ADMIN_OPTIONAL) PLUS an explicit admin_api_key check -- the sgl-project#510 regime with the belt-and-braces the most sensitive routes should get -- reaching attach/detach/resize_storage_backend on the tree cache via scheduler handlers at scheduler.py:7625/7681/7728. The semantics the brief specified are the semantics implemented: attach/detach refuse a non-idle scheduler BY NAME ("Reject attach: scheduler is not idle. #queue-req=... #running-req=..."), and resize-down evicts inline and "returns once usage is back under the new cap", with in-flight (reserved but uncommitted) writes never evicted -- so it does not truncate live pages, and the write interlock is _pending_writes rather than a race with the backup queue. resize deliberately does NOT require idleness, which is a narrower and better interlock than attach/detach's whole-scheduler gate. RETRACTION. I wrote a hermetic pin file on the finding that "resize has no coverage at all", having grepped for resize INSIDE the E2E attach/detach test. Wrong: test/registered/unit/mem_cache/test_hicache_runtime_resize_545.py exists with 21 tests covering every property I pinned and several I did not -- grow-evicts-nothing, shrink-until-under-cap, LRU victim order, enable-at-runtime-adopts-existing-files, in-flight-write-not-evicted, lifting-the-cap-disables-eviction, non-owner-MLA-rank-inert, the request validation layer, and both cache classes. My file was 100% duplicate and is DELETED; shipping it would have created a second authority for the same properties, which is what I refused in sgl-project#536. Third instance of the same error (after sgl-project#726 and sgl-project#677): concluding absence from the file I happened to open instead of grepping for the thing itself. "No coverage" requires a search FOR the coverage. My harness also passed max_size_bytes/min_free_bytes as extra_config keys when the real names are max_size/min_free_space, so every evictor it built came up UNCONFIGURED and most pins still passed -- a harness passing for the wrong reason. TWO MORE BRIEF PREMISES THAT DO NOT EXIST. MixedLayoutError: zero hits across python/. The cited commit 19f4c68 describes it in its message but no such class is in the worktree; the nearest relative guards PD draft-KV, not HiCache attach. What attach actually uses is a same-backend check (hiradix_cache.py:544-568) refusing a DIFFERENT backend by name. ReadBufferPool / sgl-project#720: also absent, no class and no reference. I did not design around either guess. PRIOR ART the brief did not mention: docs/dev/NOTE_544_hicache_runtime_ preserve_thinking.md is a desk-complete investigation of this same ticket, and docs/advanced_features/hicache_storage_runtime_attach_detach.md is the user-facing documentation of the shipped feature. THE ONE REAL GAP, and it lands on this rig's model family: UnifiedRadixCache.attach_storage_backend and detach_storage_backend (unified_radix_cache.py:2769,:2783) are HARD STUBS that always fail -- "does not support runtime HiCache storage attach yet". resize works there; attach and detach do not. UnifiedRadixCache is what registry.py:191 constructs on the path that appends the MAMBA component for is_hybrid_ssm, so for the hybrid-GDN family the shipped story is resize yes, attach/detach no -- the ticket's headline capability is exactly the half that is stubbed. I did NOT verify which cache class our specific boot instantiates; that needs the boot config, not the source. Also NOT ESTABLISHED: no test raises or mocks a real OSError(ENOSPC); disk-full is covered only through the min_free_space watermark refusal. RECOMMENDATION: implement UnifiedRadixCache attach/detach (a real implementation task -- the stubs exist because that cache's controller lifecycle differs from HiRadixCache's), then a real ENOSPC injection test. Live-window acceptance filed in the note, including that attach and detach are expected to fail on a hybrid-GDN model with the named refusal -- that is the acceptance for the gap, not a second bug report. Existing suite re-verified: 21 passed. codespell clean. Desk only.

…hybrid-GDN family stops being resize-only ANCESTRY CHECK FIRST, as asked, and it passes. The 5 routes and the 21-test resize suite both come from 470b88e (2026-08-04), which IS in this branch's ancestry AND on the serving line 5fed8a6. No train branch touches unified_radix_cache.py, and MERGE_TRAIN_2026-08-17 lists fix/717-rebuild as merging CLEAN (held for its own review boot). So the sgl-project#545 conclusions were not missing train-carried changes. PREMISE: WHY THE STUBS EXISTED. The open question was whether attach on this cache is a GROUP operation. It is: _symmetrize_prefetch_capacity enters an all_reduce across DCP/TP ranks to derive the prefetch limit from the MIN host pool, and its own guard refuses a rank-local early return because that "would leave the other ranks in the all_reduce with no partner". A single-rank attach would hang. That turned out NOT to be a blocker. attach_hicache_storage fans out through FanOutCommunicator (tokenizer_control_mixin.py:125,:360) to EVERY rank and merges the results, so the group runs it together, and the scheduler refuses a non-idle scheduler by name first. The stubs were unimplemented, not blocked. BUILT, mirroring the shipped HiRadixCache pair: same validation with no side effects on refusal, same named refusals, same "same backend is success, a DIFFERENT backend is refused rather than silently swapped" rule (a swap would strand every page written under the old backend), plus _symmetrize_prefetch_capacity AFTER the config is applied -- the limit is the cross-rank MIN and can only be derived once each rank knows storage is on. DETACH ORDER IS THE CONTRACT, taken from HiRadixCache: drain the control queues BEFORE tearing the controller down, or acks and releases can no longer be matched to their nodes and host pages and locks leak; drain again after to sweep what the shutdown produced. The drain is LOCAL (None limits = everything on this rank) because the steady-state path derives its counts from an all_reduce and a detach may not depend on a collective its peers may already have left. A detached tier's pages become MISSES, never corruption -- keys are content-addressed, the same argument sgl-project#703 uses for drops. NO SILENT PARTIAL CAPABILITY (sgl-project#268): the MAMBA/state component is COVERED, not refused. _get_hybrid_storage_attach_kwargs passes cache_controller.mem_pool_host.entries, which spans every component the controller owns -- the same set the boot-time attach passes -- so a hybrid attach is whole rather than KV-only wearing a success message. Pinned. Tests, hermetic (CUDA_VISIBLE_DEVICES="", no server, no CUDA): test_unified_attach_detach_545.py 16 passed -- SIBLING to the resize file, not merged into it: that file owns the resize authority, one authority per behaviour (my own sgl-project#536 rule). RED-FIRST: restoring the stubs fails 15 of 16. + the existing resize suite: 37 passed together. test/registered/unit/mem_cache/ 940 failed / 819 passed -- the 940 are the unchanged pre-existing baseline on this base (lacks the sgl-project#585 conftest); passes moved 803 -> 819, exactly the 16 new pins. No regressions. ruff + codespell clean. ACCEPTANCE UPDATED in NOTE_545: attach/detach on a hybrid-GDN model are now EXPECTED TO SUCCEED with the state component covered; the previous "expect the named refusal" acceptance is struck through as superseded. STILL OPEN, named not fixed: the ENOSPC injection test, and tokenizer_control_mixin.py:370's own "# TODO: partial rollback if failed" -- if some ranks attach and others do not, nothing rolls back. The fan-out merges results so the caller learns it failed, but the group can be left inconsistent. That gap predates this change. Desk only, no boot, no model load, no serving contact.

…tach, and ENOSPC injection 1. PARTIAL ROLLBACK (the pre-existing TODO at tokenizer_control_mixin.py:370). A mixed fan-out result left the group HALF-ATTACHED -- some ranks running storage threads with a backend bound, others not -- and reported it as a clean failure. That is exactly the state the detach contract exists to prevent. Now the terminal state is all-attached or all-detached, never mixed: on a mixed result the coordinator detaches the group, and if that rollback fails anywhere the response NAMES the stranded ranks rather than reporting clean failure. Three things it needed: * A RANK ON THE REPLY. FanOutCommunicator.handle_recv appends results in ARRIVAL order, so list position does not identify a rank -- "ranks 0 and 2 are stranded" was literally unsayable. Attach/Detach outputs now carry `rank` (flat world rank pp_rank*tp_size+tp_rank), stamped in a WRAPPER so every return path gets it; stamping at each `return` would let a later-added path ship unstamped (-1) and silently break the naming. Uses self.ps.tp_rank, NOT self.tp_rank -- the Scheduler keeps its parallel identity on the ParallelState wrapper and the bare attribute does not exist (the file's own comment records that trap). * A GROUP DETACH, not a targeted one: detach is idempotent by construction (it asks the controller to clean up even when enable_storage is already False, precisely to sweep partial-attach leftovers) and the communicator offers no rank-addressed send. * COLLECTIVE-FREE, per this ticket's own rule: the detach path drains with local (None) limits and enters no all_reduce, so a rank whose peers have already left a collective cannot hang on it. The verdict is NEVER flipped to success; rollback only changes what the message can tell you. 2. ENOSPC INJECTION at the syscall boundary. Technique reused from the canonical store's three-site injection; none of its code, which lives on an unmerged train branch. Pinned: a write hitting ENOSPC returns False AND the key is genuinely not readable (False is only honest if the page is really absent, or the tree believes a page is backed and the next prefetch resolves to nothing); no torn page is ever visible, because the page appears only at os.replace -- a failure before it leaves the final path absent, a failure at it leaves prior content intact; the watermark REFUSES AT THE DOOR when a cap is configured rather than failing mid-write with the IO already spent; and the tier still works after. ONE PIN HAD TO BE REBUILT TO BE HONEST. The reservation-leak check first asserted that a later write still fits after five failed ones. It COULD NOT FAIL: with a cap in force the evictor simply EVICTS to admit, so a leaked reservation causes extra eviction rather than a refusal, and the pin passed with `abort` removed. It now asserts on _total_bytes directly, which is what distinguishes released from leaked. The mutation that previously failed 1 pin now fails 2. Tests, hermetic (CUDA_VISIBLE_DEVICES="", no server, no CUDA): test_attach_partial_rollback_545.py 9 passed MUTATION: dropping the rollback fails 4, including the mixed-state pin test_hicache_enospc_545.py 11 passed MUTATION: removing abort + tmp cleanup fails 2 test/registered/unit/managers/ 13 failed / 2217 passed -- failures unchanged from the 13-failure baseline, passes 2208 -> 2217, exactly the 9 new pins test/registered/unit/mem_cache/ 940 failed / 830 passed -- failures unchanged from the pre-existing 940 baseline (this base lacks the sgl-project#585 conftest), passes 819 -> 830, exactly the 11 new pins ruff + codespell clean; scheduler.py ruff count unchanged at its pre-existing 94 (verified against the HEAD version). Branch head moves to this commit; the merge-train doc is not carried on this branch (it lives in the wt-reconcile / wt-706 worktrees), so the new head is recorded here rather than by editing another strand's tree. Desk only, no boot, no model load, no serving contact.

ZackZeng999 changed the title ~~Zeng~~ Some Issues in Test Jun 14, 2024

Ying1123 changed the title ~~Some Issues in Test~~ Update test cases Jun 14, 2024

Ying1123 self-assigned this Jun 14, 2024

Ying1123 requested changes Jun 15, 2024

View reviewed changes

Comment thread test/srt/model/test_llama_low_api.py Outdated

Comment thread test/srt/model/test_llama_low_api.py Outdated

Qubitium reviewed Jun 18, 2024

View reviewed changes

Comment thread test/srt/model/test_llava_low_api.py Outdated

Ying1123 force-pushed the zzw2 branch 3 times, most recently from 3431d94 to 600c55f Compare June 30, 2024 06:39

Ying1123 changed the title ~~Update test cases~~ Minor fix in compiler & format Jun 30, 2024

fix tests

0c2c396

Ying1123 force-pushed the zzw2 branch from 600c55f to 0c2c396 Compare June 30, 2024 06:41

Ying1123 merged commit 11616fc into sgl-project:main Jun 30, 2024

Ying1123 linked an issue Jun 30, 2024 that may be closed by this pull request

ImportError: cannot import name 'pin_program' #532

Closed

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

Minor fix in compiler & format (sgl-project#545)

8d182ac

Garrybest pushed a commit to Garrybest/sglang that referenced this pull request Jan 9, 2026

feat: support parallel slice model loader (sgl-project#545)

d4fbd4a

* parallel model model

michaelzhang-ai mentioned this pull request Feb 21, 2026

[AMD][DO NOT MERGE] Fix MI35x DeepSeek-V3.2 nightly timeout failures #19110

Closed

5 tasks

Navigation Menu

Uh oh!

Minor fix in compiler & format - #545

Minor fix in compiler & format#545
Ying1123 merged 1 commit into
sgl-project:mainfrom
ZackZeng999:zzw2

ZackZeng999 commented Jun 14, 2024

Uh oh!

Qubitium commented Jun 14, 2024

Uh oh!

ZackZeng999 commented Jun 14, 2024

Uh oh!

Ying1123 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ZackZeng999 commented Jun 14, 2024

Uh oh!

Qubitium commented Jun 14, 2024

Uh oh!

ZackZeng999 commented Jun 14, 2024

Uh oh!

Ying1123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants