[Config] One writer for the declaration stash; no exception to the write seal - #38752
Merged
Merged
Conversation
Four functions wrote that stash, three of them by hand, and two of them carried their own copy of the same published-instance guard with different wording. Now `declare_resolution` is the only writer and the only place the guard lives. - `run_post_process_pass` and `declare_direct_writes` declare through it instead of appending to `_resolved_overrides` themselves. - `declare_late_resolution` loses its duplicate guard and a `_runtime_mutations` log that nothing in the tree ever read; its body is now one delegating call. - The guard moved up, so a declaration on the published config is refused whoever makes it. One exception, which the pass slot already documented and I nearly broke: a pass returning an *empty* dict is a validation, not a declaration, and has to keep working on the published instance because `Engine(server_args=sa)` after `Engine.shutdown()` re-runs `check_server_args` on the very instance the context holds. Only a non-empty return reaches the guard. `declare_late_resolution` stays as a name, which is a correction to what I proposed. I was going to fold it away and let location carry the launcher-stage distinction -- and measuring says location cannot: `lora_hook` is late and lives inside `arg_groups/`, while the NPU default helper (nine fields) and the expert-pack loader are not late and live outside it. Two guardrails read that distinction by name (`_declared_by_late_resolution`, `_late_resolution_written_fields`), so the name is load-bearing even though the behaviour is not. It is two lines now and says why it exists. Separately, `resolution_projection` is gone. It had one caller -- `ServerArgs.resolved_dict`, whose body was `return resolution_projection(self)` -- and both carried a docstring explaining the same operation, which is most of why this module is hard to read. The projection and its `_plain` helper moved into the method; `resolved_dict` keeps its seven callers.
`ServerArgs.replace_resolved` copied a resolved record so the copy could carry one changed field across a process boundary. None of its three callers needed a copy. - `ray/scheduler_actor.py` gets its record as a Ray method argument, so it is the actor's own unpickled instance and nothing else in that process holds it. - `ray/engine.py` hands the record to the DP controller in-process; that process does not publish here, and the only local reader of the field is `PortArgs.init_new`, which reads the decision. - `benchmark/one_batch.py` reassigned over its own variable and dropped the original. So all three declare on the record they hold. Measured before changing anything: declaring `dist_init_addr` on a record that has already resolved reaches `resolution_result` and the bag the receiving process projects, the parent is untouched, and -- unlike the copy -- the *field* stays the operator's input. `replace_resolved` went through `dataclasses.replace`, which wrote the field and then declared it too, leaving the record no longer a record of what was typed. Cheng asked whether a bag override could stand in. It cannot, and my first reason was wrong: I said overriding needs a publish, and it does not -- `set_server_args` is what projects the bags, `publish` only adds the role, and `override` works as soon as the context holds a record. The actual reason is narrower: `override` writes bag leaves and by contract never touches the record, so its effect cannot travel inside an object to another process. Tests: two of the five cases pinned the copy mechanism itself -- that a copy stays resolved and that it carries the `model_config` memo, the bug that once killed the Ray schedulers. With no copy in `srt` (checked: no `dataclasses.replace` of a record left in `server_args.py` or `arg_groups/`) that failure class is gone by construction rather than untested, so those two go. The three that pinned real properties are rewritten onto the surviving mechanism, including the Ray hop end to end: pickle, declare, publish, and assert the bag moved while everything resolution decided stayed put.
… skill `declare_direct_writes` -> `capture_foreign_writes`. It was the odd one in the `declare_*` family: not a declaration channel, because the writer is not ours. An out-of-tree platform plugin or a registered speculative algorithm is handed the record and sets fields on it -- an interface this tree does not own -- so the write is observed and declared rather than requested. It is also the one place the write seal comes off, which the old name hid. Measured while deciding whether to delete it instead: across every entry in the resolution matrix it is invoked eleven times and captures a field zero times, because `Platform.apply_server_args_defaults` is `pass` with no in-tree override, `CustomSpecAlgo.handle_server_args` is `pass`, and the in-tree speculative dispatcher declares properly. So the machinery is entirely out-of-tree-facing. Kept anyway: deleting it changes a documented plugin contract, which Cheng ruled out. The skill doc catches up with this branch: the declaration stash now has one writer and a table of the spellings around it, the copy-a-resolved-record passage is replaced by what the Ray paths actually do, and the note about `override` needing a publish is corrected -- it does not, `set_server_args` is what projects the bags. Checked every API name the doc names against the package; the only two that do not resolve are the two it says are gone.
… now Cheng asked twice why this name had to stay and was right both times. My argument was that two guardrails scan for it, so the name is load-bearing -- which had it backwards: the guardrails are ours, and what they need decides whether the name does. They need *coverage*, not the distinction. Both consume the late fields as a union term (`written |= ...`, `... | _declared_by_late_resolution()`), and the chain ratchet's two assertions about them -- a floor of three, and `by_late <= by_keyword` -- only ever proved the late scan itself still ran. The reason the two collectors both existed is smaller than it looked: they are keyed on different *names*. `_hook_assignment_targets` scans `arg_groups/` for `declare_resolution`, so it could not match `declare_late_resolution`, which is why `lora_hook`'s fields reached the census only through the late scan. Renaming the call sites makes it pick them up -- checked directly: `lora_paths`, `lora_target_modules` and `enable_lora` all land in `_hook_assignment_targets` afterwards. So: nine call sites renamed, the function deleted, and the two collectors reduced to one keying each. The exposure ratchet's late collector becomes `_declared_outside_the_pipeline` -- same job, expressed as "a declarer outside `arg_groups/`", which also covers the NPU default helper and the expert-pack loader that were never late and never in it. `arg_groups/` is excluded on purpose: `_hook_assignment_targets` covers it exactly and resolves the pipeline's computed `**` expansions, which this collector's resolver cannot read. The chain ratchet's late channel is gone; `_DECLARERS` already scanned the whole tree for both names, so its keyword scan lost nothing. Guarded against the failure this ratchet's own docstring names -- a silently shrinking written set. Measured from a worktree at the previous commit rather than by stashing: nothing removed, one field added (`dist_init_addr`, from the Ray declarations that `replace_resolved` used to hide behind a hand-written stash append, invisible to every collector), and reads-of-a-written-field stays at zero. The chain ratchet's own set is unchanged at 152.
…d-in Cheng's read was right: this was an override of server args wearing a capture's clothes, and the platform half belongs beside the platform rules it sits next to. `capture_foreign_writes` lifted the write seal, let a plugin assign fields on the real record, then diffed the record to find what moved. It is now `record_foreign_defaults`, which hands the plugin a stand-in: reads fall through to `resolving_view`, writes are captured, and what it assigned is declared. Three things follow. The seal has no named exception any more -- `record_writable` is deleted, and so is the one place the record was writable during resolution. A plugin also now reads what resolution has decided so far rather than the raw fields, which is what every in-tree rule reads. And nothing is inferred from a diff, so a plugin assigning a value identical to the one it read is still a declaration. The platform call becomes `handle_platform_defaults` in `platform_hook.py`, beside `handle_npu_backends`, `handle_amd_specifics`, `handle_xpu_backends` and the rest -- one shape for platform rules, in-tree or not. The two speculative sites are split by type, which is what let the wrapper go from three call sites to two. `SpeculativeAlgorithm.handle_server_args` is the in-tree dispatcher: measured with AST, its six `_handle_*` targets make zero field writes and thirty-three declarations, so it needs no capture at all -- and it must get the record itself, because handed the stand-in its `declare_resolution` calls would stash on that. `CustomSpecAlgo` is the out-of-tree shape and gets the stand-in. `isinstance` separates them cleanly: one is an `Enum`, the other is not a subclass of it. Two things caught while doing it, both by running rather than reading. The `CustomSpecAlgo` import is bound inside the `speculative_algorithm is not None` branch, so my first split raised `NameError` for every run without speculative decoding. And `get_platform()` is the facts view, which carries neither `device_name` nor the hook -- the plugin object is `platforms.current_platform`. Coverage checked, not assumed: the exposure ratchet's written set is unchanged at 123 with nothing removed, and reads-of-a-written-field stays at zero. The platform-plugin test now patches the platforms module rather than the pipeline, since the hook resolves `current_platform` when it runs; it passes, which is the end-to-end proof that an assigning plugin still reaches the projection. `test_fast_prefill_plan.py`'s two failures predate this branch.
ch-wan
force-pushed
the
cheng/config-api-cleanup
branch
from
September 9, 2026 23:39
b0ec34f to
bf050be
Compare
pllimax
added a commit
to pllimax/sglang
that referenced
this pull request
Sep 10, 2026
* origin/main: (27 commits) [Simulator] Give the OFFLINE/BLOCKING comparison tolerances real headroom (sgl-project#38732) [Config] msgspec.Struct for the config tier (sgl-project#38753) [AMD] ci: move the miles nightlies from rocm700 to rocm10 (sgl-project#37495) [Config] One writer for the declaration stash; no exception to the write seal (sgl-project#38752) docker(xpu): drop redundant setvars.sh from torch_memory_saver RUN (sgl-project#38665) [XPU][Fix] Pack device-pointer tables as uint64 to avoid 64-bit address overflow (sgl-project#35051) [CI] Temporarily disable GB300 tests (sgl-project#38770) [diffusion] feat: spill large tensors over shared memory like numpy arrays (sgl-project#38656) [diffusion] refactor: refactor utility ownership and document helper placement (sgl-project#38699) [NPU]Support GLM5.2 and FP8 DSA&Indexer kvcache for 950 (sgl-project#38250) [CI] Answer unrecognized slash commands instead of skipping silently (sgl-project#38736) [AMD] Parallelize aiter spec-decode KV index building over token blocks (sgl-project#37659) [DSv4] Integrate TRT-LLM DSv4 Attention for SM100/103 (sgl-project#30805) Add Opt-In for GLM-5.3 Flash breakable prefill CUDA graphs (sgl-project#38522) [CI] Install helion 1.4.0 for the KDA Helion kernel tests (sgl-project#38688) [Rust] Gate health on startup warmup completion (sgl-project#37994) [HiCache] Replace skip_lock_node_ids with a segment lock protocol (sgl-project#36848) feat: add optimized Domino rollout to DFlash V2 (sgl-project#36899) [CI] Add /run-full-ci and /run-extra-ci slash commands (sgl-project#38734) [Model] Support GLM-5.3 Flash NVFP4 loading (sgl-project#38621) ...
5 tasks
mqhc2020
pushed a commit
to mqhc2020/sglang
that referenced
this pull request
Sep 15, 2026
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.
Five functions in
server_args.pyandarg_groups/overrides.pywere hard totell apart --
declare_late_resolution,declare_direct_writes,replace_resolved,resolution_projectionand the pass slot all wrote thesame declaration stash, three of them by hand. Two carried their own copy of
the same guard, with different wording. This makes one call the only writer.
declare_resolutionis the only writer, and the only guardrun_post_process_pass,capture_foreign_writesandreplace_resolvedappended to
_resolved_overridesthemselves. They declare through the familyfunction now, so "refuse the published config" -- the stash is projected at
publish and never again, so a later declaration is a silent no-op -- lives in
one place instead of two.
One exception, which the pass slot already documented and I nearly broke: a
pass returning an empty dict is a validation, not a declaration, and must
keep working on the published instance, because
Engine(server_args=sa)afterEngine.shutdown()re-runscheck_server_argson the very instance thecontext holds. Only a non-empty return reaches the guard.
declare_late_resolutionis gone. It added a name and a dead_runtime_mutationslog that nothing in the tree read. I first kept the name,arguing two guardrails scan for it -- that was backwards, since the guardrails
are ours. They need coverage, not the late/not-late distinction: both take
the fields as a union term. The two collectors existed only because they keyed
on different names, so
_hook_assignment_targets(which scansarg_groups/for
declare_resolution) could not match the late spelling, which is whylora_hook's fields reached the census only through it. Renaming the callsites makes it pick them up.
replace_resolvedis gone; the callers declareIt copied a resolved record so the copy could carry one changed field across a
process boundary. None of its three callers needed a copy: the Ray scheduler
actor gets its record as a method argument (its own unpickled instance), the
Ray engine hands it to an in-process controller whose only local reader is
PortArgs.init_new, and the benchmark reassigned over its own variable.Declaring on the record they hold is also more correct:
replace_resolvedwent through
dataclasses.replace, which wrote the field and then declaredit too, leaving the record no longer a record of what was typed.
A bag
overridecannot stand in, and not for the reason I first gave.Overriding does not need a publish --
set_server_argsis what projects thebags. The actual reason is that
overridewrites bag leaves and by contractnever touches the record, so its effect cannot travel inside an object to
another process.
The write seal has no exception
capture_foreign_writeslifted the seal, let an out-of-tree plugin assignfields on the real record, then diffed the record to find what moved. It is
record_foreign_defaultsnow, which hands the plugin a stand-in: reads fallthrough to
resolving_view, writes are captured, what it assigned isdeclared.
record_writableis deleted along with the only window in which therecord was writable during resolution.
The platform call becomes
handle_platform_defaultsinplatform_hook.py,beside
handle_npu_backends,handle_amd_specificsand the rest. The twospeculative sites split by type: the in-tree dispatcher declares (measured
with AST -- its six
_handle_*targets make zero field writes andthirty-three declarations) and must get the record itself, because handed the
stand-in its own
declare_resolutioncalls would stash on that.resolution_projectionOne caller --
ServerArgs.resolved_dict, whose body wasreturn resolution_projection(self)-- and both carried a docstringexplaining the same operation, which is most of why this module reads as
arbitrary. Folded into the method.
Testing
19 files, +334/-421.
overrides.pygoes from 23 public functions to 21;writers of the declaration stash from five to one; exceptions to the write
seal from one to zero.
Coverage measured rather than assumed, because the exposure ratchet's own
docstring warns that a silently shrinking written set passes forever: nothing
removed from it, one field added (
dist_init_addr, from the Ray declarationsthat
replace_resolvedused to hide behind a hand-written append, invisibleto every collector), and supplied-instance reads of a written field stay at
zero. The chain ratchet's set is unchanged at 152.
Two tests were deleted rather than rewritten: they pinned the copy mechanism
itself -- that a copy stays resolved, and that it carries the
model_configmemo, the bug that once killed the Ray schedulers. With no copy left in
srtthat failure class is gone by construction. The three that pinned real
properties moved onto the surviving mechanism, including the Ray hop end to
end: pickle, declare, publish, and assert the bag moved while everything
resolution decided stayed put.
The
sglang-runtime-contextskill is updated with all of it, including acorrection: it said a bag override needs a publish.
CI States
Latest PR Test (Base): ❌ Run #34417882989
Latest PR Test (Extra): ❌ Run #34417882846
Latest PR Test (AMD ROCm 10): ❌ Run #34417883039