Conversation
ch-wan
requested review from
BBuf,
Edwardf0t1,
Fridge003,
HaiShaw,
JustinTong0323,
Qiaolin-Yu,
Ying1123,
alexnails,
alphabetc1,
fzyzcjy,
hanming-lu,
hnyls2002,
huangtingwei9988,
hzh0425,
ispobock,
jybsuper,
kpham-sgl,
lifuhuang,
liusy58,
merrymercy,
mickqian,
pyc96,
sufeng-buaa,
sundar24295s,
xiezhq-hermann,
yctseng0211,
yhyang201,
yizhang2077,
yuan-luo and
yushengsu-thu
as code owners
September 5, 2026 08:46
ch-wan
force-pushed
the
cheng/gc-r6-review
branch
from
September 6, 2026 08:45
e105296 to
a6bfbe6
Compare
github-actions
Bot
added
the
documentation
ch-wan
force-pushed
the
cheng/gc-r6-review
branch
6 times, most recently
from
September 6, 2026 21:57
e068fab to
b50162a
Compare
…it means
`swa_full_tokens_ratio` and `mamba_full_memory_ratio` carried real values as
their class defaults (0.8, 0.9), so a model family with an opinion had to ask
"is this field still equal to the class default?" to find out whether the
operator had set it. That question has two wrong answers: it says "the operator
set it" as soon as any earlier pass declares the field, and it says "the
operator did not set it" when the operator types the default value.
Both become `Optional[float] = None`. The record carries what the operator typed
and nothing else, and the family test becomes `is None`.
That leaves the other half: something has to say what the field means when
nobody answers. `Arg(fallback=...)` says it in the declaration.
swa_full_tokens_ratio: A[
Optional[float],
Arg(help="...", resolvable=True, fallback=0.8),
NS("schedule"),
] = None
The dataclass default stays `None`. A fallback is not a default: the record is
the wire format, and a child process has to keep being able to tell "unset"
from "set to the value resolution would have picked anyway".
It is applied at the bottom of the *effective* read chain -- override, then
decision, then input, then this -- in `resolution_result`, which the projection,
`/server_info` and every config bag read through. Deliberately **not** in
`resolving_view` / `resolved_view`: those are the decision-over-input surface a
pass reads while it is deciding, and `model_overrides/inkling.py` and
`deepseek_v4.py` both branch on `if cfg.swa_full_tokens_ratio is None` before
declaring 0.1. A `__getattr__` layer is read-time, so a fallback answering there
is not "the generic value, later" -- there is no later, and the family branch
would never fire. Running `_inkling_overrides` both ways:
fallback on the effective surface only: family declared swa = 0.1
fallback also on the view a pass reads: family declared swa = None
So "resolution first, then the fallback" holds -- not because a step is appended
to the pipeline, but because of which surface the value lives on. The one
in-pipeline reader that wants the effective value, the range check on the ratio,
asks `resolution_result` directly; that is what its comment already claimed it
was doing, and it runs after the model families.
The alternative was a pass that fills the field in when nothing claimed it. That
needs a slot (after the families, or it beats them), a second call site (the
dummy-model short circuit returns long before that slot), an idempotence
requirement so the second call is harmless, and the value written twice -- once
as a literal, once as prose in the help. A declaration needs none of it: nothing
has to run for a field to mean what it says.
Only a value fixed for the life of the configuration belongs here. A default
that depends on the machine (`get_device()`), on another field (`tokenizer_path`
following `model_path`) or on anything impure (`random.randint`) is a decision,
and decisions stay in a hook where their order is visible. Of the 29 fields a
hook currently fills from `None`, about 20 are conditional decisions of that kind
-- seven memory tiers choosing `chunked_prefill_size`, a model family choosing
`max_running_requests` -- and they are not candidates.
`mamba_radix_cache_strategy` keeps `"auto"`: unlike the ratios it already has a
spelling for "unset" that an operator can type and that means exactly that --
only its comparison changes, from the class default to the token itself, which
is the fix the comment at that site already prescribed. With that, neither family
module imports `ServerArgs` any more.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ServerArgs` carried all 487 declarations in one 4,462-line file, each tagged
with an `NS("...")` marker naming the namespace it belongs to -- structure
supplied by annotation, in a file a namespace away from the
`arg_groups/*_hook.py` that resolves it.
The declarations move to `arg_groups/fields/`, one module per top-level
namespace and one class per leaf namespace (21 of them, `exec.moe` becomes
`exec_.py::ExecMoe`). The class carries the `_NS_PATH` it stands for, so the
module a field is declared in *is* its namespace and the marker is redundant:
`namespace_of` reads the declaring class off the MRO instead. `NS` stays for
the one case a class cannot express -- a single ad-hoc dataclass whose fields
span namespaces, which is what the config-bag tests build.
`ServerArgs` composes the 21. It is still one flat dataclass with 487
attributes, so `server_args.tp_size`, `ServerArgs(model_path=..., tp_size=8)`,
pickling to a subprocess and all 408 parameter sites are untouched. `Model` is
the last base because reverse-MRO order puts the last base's fields first, and
`model_path` -- the one field with no default -- has to stay the first
positional argument.
Two things travel with the declarations. The twelve `*_CHOICES` lists and the
`add_*_choices` adders that extend them move to `arg_groups/choices.py`, since
the fields naming them can no longer import from `server_args` without a
cycle; `server_args` re-exports them for the plugins that have always reached
them there. And five fields whose only annotation element was the namespace
marker become plain annotations -- `A` is `Annotated`, which needs two
arguments, so stripping the marker would have left them invalid.
Verified three ways: the 487-entry `namespace_of` map is identical field for
field, the resolution result is identical across 24 launch shapes x 485
fields, and the CLI registers the same 503 options with the same defaults,
choices and actions.
Ten places still told the reader that a field's namespace comes from an `NS(...)` marker on the field. Two of them are errors a developer reads when something is misconfigured -- "no NS namespace" would send them looking for a marker that no longer exists on any ServerArgs field. The namespace now comes from the class that declares the field, which is what `namespace_of` reads, so the messages name the namespace rather than the mechanism that used to supply it.
The split re-exported the names the extension-point block defines. That misses a list the block never mentions: `SAMPLING_BACKEND_CHOICES` is read only by the CLI section, so `add_cli_args` raised `NameError` on the first call -- which no import-time check catches. Re-export everything `choices.py` defines. All 33 used to be importable from `server_args`, and a rule narrower than that has no way to know which ones a plugin reaches for. The typing names the moved declarations were the last users of go the other way and are dropped.
Inheriting the namespace classes made the record's contents a property of which classes happen to appear in a base list. That is correct only while every namespace declares nothing but operator input, and it stops being correct the moment a derived field is declared -- `attn_tp_size` belongs in `parallel.py` next to the leaves it is derived from, and inheriting `Parallel` would put it on the record, where it is neither input nor safe: the record is the wire format, so a derived width pickled to a subprocess is a stamp, and a width has to stay a live read for elastic scale-up to see the group it has. `collect_input_fields` takes the classes that declare input and returns their annotations, defaults and namespaces. Each source's annotations are resolved in its own module and handed on as type objects -- carried across as text they would be re-evaluated where they land, and the composing module deliberately imports none of the names the declarations use. So a namespace can now declare both halves side by side, and which half reaches the record is one readable call instead of an invariant spread across a base-class list. Nothing is registered on the derived side yet; this is what makes it possible. The order is the old reverse-MRO order, so the fields, the CLI's 504 options and `model_path` as the first positional argument are all unchanged. Verified the four ways this refactor can be: the 491-entry namespace map and the CLI surface are identical to origin/main, the resolution result is identical across 24 launch shapes x 489 fields, and nothing that was importable from `server_args` stopped being.
`/server_info` reports `resolved_dict()` -- what resolution decided. There was no way to ask the other question: what did the operator actually type? The two are not derivable from each other, and the resolved side cannot stand in for the raw one, because a field nobody set reads the same as one set to the value resolution would have picked anyway. The launcher stores the arguments it parsed and the in-process `Engine` stores the call that built the record; `/server_info` and `Engine.get_server_info` report it beside the resolved values, so both surfaces are answerable from one request. It rides on the record rather than in a field: it describes how the configuration was asked for, so it is not part of the configuration -- no CLI flag, no namespace, not in the bags. Being on the record is what lets a subprocess copy answer the same question the launcher can, and `replace_resolved` carries it because a copy was launched by whatever launched its parent.
The read-only guard armed on `_resolution_finished`, so for the whole run of the pipeline nothing stopped a resolver from assigning a field. Nothing in `srt/` does -- 0 assignments statically, and 0 writes observed across the launch-shape matrix with a watching `__setattr__` -- but that was a convention, and the defect it permits is invisible: a value a resolver wrote onto the record is indistinguishable from a value the operator typed, which is the one distinction the record exists to preserve. Arm it when resolution starts instead. A resolver that assigns a field now fails at boot with a message naming `declare_resolution`, which is where the decision belongs: the stash carries a source and leaves the input intact. `declare_direct_writes` asks for the seal by name through `record_writable`. It hands the record to an out-of-tree platform plugin that sets fields on it; those implementations cannot be converted by editing a resolver here, so the write stays and the diff is captured into the stash afterwards. Naming the exception is the point -- an in-tree resolver reaching for it is doing something it should be declaring. Costs nothing: the 211 test-side assignments all happen before `resolve_once`, which a post-resolution write already refused.
The record is the operator's input; the bags are what is in effect. A reader that takes the record and reads a field off it gets the input, which is the wrong one of the two whenever resolution decided something -- and the mistake is silent, because for most fields and most launches the two agree. These seven already read both ways, sometimes in the same expression: `get_tokenizer(get_serving().tokenizer_path, tokenizer_mode=server_args.tokenizer_mode, ...)`. Every read here runs after its process publishes -- the two subprocess entry points publish before anything else, and the engine's own reads all sit below the launcher's publish -- so each one is a read of the same value from the surface that owns it. The parameters stay. Removing them is a signature change on call chains that reach constructors other implementations override, which is a separate decision from where a value is read.
Converting the reads left `_resolve_backend`, `_set_all_reduce_flags` and `_compute_parallelism_ranks` taking a record they no longer name. The dead- parameter ratchet is what noticed; the parameter and the argument go together at every call site. `_resolve_backend` shares its name with an unrelated function in `flashinfer_comm_fusion`, whose own callers and tests are untouched. Their callers keep theirs: `init_torch_distributed` still hands the record on.
Sixty-odd more files took the record and read config off it. Every one of them runs after its process publishes -- the launcher's own reads sit below `_launch_subprocesses`, the two subprocess entry points publish first thing, and the serving and model-executor layers only exist afterwards -- so each is the same value read from the surface that owns it. Two findings worth keeping. Eleven reads were `getattr(record, "field", default)`, which an AST scan for attribute access does not see: the census that said "43 readers" was counting the shape it could match, not the thing it was after. `incremental_streaming_output` was read that way twice, and the transcription tests were the only reason it surfaced. And not every record read is a bag read waiting to happen. A multimodal processor's `base_gpu_id` is the instance's, not the process's: two engines in one process keep different ones, and `test_publishing_another_config_does_not_move_the_device` exists to say so. That one stays on the record, while `rl_on_policy_target` beside it moves -- the test suite is what drew the line. The fixtures move with the code. Tests that hung config off a mock manager now publish a record, which is what the serving layer reads; where a test states a value, it says so with `override_server_args` instead of assigning through the mock.
… didn't The sweep caught what the file-scoped runs did not. Six functions were left holding a record they no longer name -- the ratchet names them -- and five test files drove code that now reads the bags without publishing anything, so the first read failed closed. Two reads go back to the record. `RequestMetricsExporter` is handed the directory it writes to at construction, and a test builds several with different ones; reading the process's value instead would make them the same exporter. That is the same line the multimodal processor's `base_gpu_id` sits on: a value one object owns is not the process's to answer for.
The field split gave every namespace a file, but only for the half an operator types. The six parallel quotients -- `attn_tp_size` and its siblings -- were sixty lines of near-identical properties in the runtime context, a file away from the leaves they are quotients of, so reading `parallel.py` told you what you could set and nothing about what that decides. They are declared in `Parallel` now, in the same class as those leaves. They carry no annotation, so they are not dataclass fields and `collect_input_fields` never puts them on the record -- the same mechanism that already keeps `_NS_PATH` off it. That is the right exclusion: a quotient has no operator input to preserve, and the record is what crosses a process boundary, so a width put there would be a stale copy the moment an elastic scale-up restamps one. A declaration names the field and says what it means, and nothing else. What a quotient *is* is a function of the leaves beside it, computed by `derive_parallel_widths`; how a value that moved after publish reaches a reader -- the stamp, and the live group below it -- is a property of the reading, so that table stays in `runtime_context` where `_derived_width` uses it. `ParallelContext` installs a property per declaration instead of carrying its own list, so the two cannot drift; a test asserts the declared set is exactly what `derive_parallel_widths` returns, and that none of them is a record field. Properties rather than `__getattr__`: these are read inside compiled model code, where an attribute load is traceable and a dynamic lookup is not.
The same question was asked three ways: a `ServerArgs` member for the resolution pipeline, a `runtime_context` function for readers after publish, and the helper both delegated to. Three places to keep saying the same thing, and a test whose whole job was to assert that two of them agreed. It is declared now, in `ExecMamba`, beside the strategy it reads. A `Derived(fn=...)` is a pure function of the published configuration, so `publish` computes it once and stores it as an ordinary bag leaf: readers get a plain attribute load, which is what a read inside compiled model code needs. The function is handed the whole resolved config rather than the bag it lands in, because a derivation is free to span namespaces and this one does -- it reads `memory.disable_radix_cache` alongside its own strategy, which is why it could never have been a method on either bag. The helper stays: resolution needs the predicate before there is a bag to read. The other two spellings and their thirty call sites go. Four test files built runtime objects that read a parallel quotient without stating a topology. They used to get one for free -- an uninitialized process group answered with defaults, which is the fallback the previous commit removed. They publish a config now, which is what a real process does. Fixing `test_multi_ended_allocator` at the source also cleared two failures it already had.
Same shape as the mamba one, three more times. `is_ep_joiner`, `is_ep_scale_joiner` and `is_startup_weight_load_overlap` each existed as a `ServerArgs` property for the resolution pipeline and, for the first two, again as a `runtime_context` function for readers after publish. They are declared where their leaves are -- the elastic-EP pair in `ExecMoe` beside `ep_join_mode`, the overlap flag in `Model` beside `startup_weight_load_mode` -- and computed into their bags at publish. Their twenty-eight read sites become bag reads, and one parameter the conversion emptied goes with them. Three readers keep the pre-publish helper instead, because they run before their process publishes: `initialize_dp_attention`, which the weight-cache daemon calls while building its groups thirty lines before its `publish`, and `PortArgs.init_new`, a factory that is handed the record and already reads eighteen other fields off it. The startup-schedule test stated the mode by standing in a namespace with the predicate on it. That is inert once the predicate is a bag leaf, so it states the mode by publishing a record with it -- which is also what the surrounding tests were already asked to do. While counting the read sites: eighteen of them read the predicate without calling it. That is correct -- they are properties -- but a census that assumes otherwise reports eighteen always-true conditions, so it is worth saying they were checked and are fine.
Rebasing onto sixty-four commits of `main` brought in tests written against the older reading. Eight files built runtime objects that now read the bags, and did it by hanging values off a mock: `scheduler.server_args.skip_tokenizer_init`, a `SimpleNamespace(cuda_graph_config=...)`, namespaces carrying a disaggregation flag, a dLLM algorithm, an external-linker backend. Those are inert once the value is a bag leaf. They publish a record carrying what they state, and where one test wants a different value it says so with `override_server_args` rather than assigning through the stand-in. Two are pytest functions rather than cases, so they get `setup_function` instead of `setUp`. Two needed more than a bare record. The graph-capacity test wants specific cuda-graph bounds, and a dummy model path returns from resolution before those are parsed, so it builds the `CudaGraphConfig` itself; the decode-radix tests want the radix flag on, which is not its default.
Four record reads arrived with sixty-four commits of `main`, written in the older style: three in `serving_chat`, which already reads ten fields out of the bags, and one in the UMBP linker. Leaving them makes the same file answer the same kind of question two ways, which is the state this series exists to remove. Both run well after their process publishes. Their tests state the values with `override_server_args` rather than assigning through the mock manager, for the same reason as the rest.
The re-export block added for the names the moved declarations used duplicated twelve imports the file already had -- those names were imported *for* the declarations in the first place. Harmless at runtime and invisible to the repo's lint config, but it is twelve names imported twice, and the second copy is the one a reader has to reconcile. All twelve keep only the annotated re-export. The plain imports go: after the split there is no code in this file that uses them, which the lint confirms by removing them the moment the `# noqa` is not on the line.
`_derived_width` answered from a stamp or, failing that, a live process group. The group read could never disagree with the stamp: `initialize_model_parallel` stamps all six as its last statement, unconditionally; an elastic scale-up restamps `attn_dp_size` through `update_dp_attention_post_scale` -- the comment claiming it does *not* was wrong; no hardware backend builds groups of its own; and `multimodal_gen`, which has its own `initialize_model_parallel`, stamps in `_sync_srt_tp_group` -- the moment it lends its TP group to `srt` as `_ATTN_TP`, which is what makes shared `srt` layers work inside it. That last one was wrong when this commit was first written: the claim was that `multimodal_gen` never reads a quotient. It does, through code it does not own -- `srt/layers/attention/vision.py` builds a `VisionAttention` and asks for `attn_tp_size` -- so a census of `multimodal_gen/` for `get_parallel()` finds nothing and the read happens anyway. It surfaced as a hard failure in `multimodal-gen-component-accuracy`, and the stamp above is the fix: this package publishes no `srt` config, so the widths have to be stated where the group is. So the group read goes, and with it the last reason for a quotient to be resolved on every read. Every input to `derive_parallel_widths` is a record field -- `dcp_enabled` is `decode_context_parallel_size > 1`, not a fact about a built group -- which is the same test the config-derived predicates in this branch pass. The six are declared the same way, `Derived(fn=...)`, and computed the same way: once, at publish, into ordinary bag leaves. What remains is override, else stamp, else the published leaf. The stamp stays above the leaf because a scale-up restamps `attn_dp_size` after publish; the override stays on top because that is how a test names a width. Overriding a leaf no longer moves its quotient: `override(tp_size=2)` leaves `attn_tp_size` where the published config put it, because nothing is recomputed on read. A test states a topology by publishing a config, which is what a real process does, or by naming the width it wants. Six tests say it that way now, and one pins the new rule directly. Four tests pinned the group read, with mocks that built "a group exists but nothing stamped it" -- a state no path produces. Two of them tested a width and a rank the same way, because both used to read the group; they do not. `attn_dcp_rank` still reads the group, gated on a width that no longer does, and `SIZE_RANK_DELEGATIONS` splits along that line. The replacement patches the group getter to raise, so "do not consult the group" is pinned rather than merely unpinned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.claude/rules/modify-component-must-read.md` points at this skill before anyone touches these files, so a stale sentence here is a wrong instruction, not a stale note. Four of its load-bearing statements stopped being true across this series: - **"every `ServerArgs` field carries `NS(...)`"** -- none do. A field's namespace is the `arg_groups/fields/` class that declares it, through that class's `_NS_PATH`; the marker survives only for an ad-hoc dataclass spanning namespaces, which is what the config-bag tests build. The same sentence was the docstring of `test_server_args_namespaces.py`, which is fixed here too. - **the DCP degrade rule** -- "before dist init a live size read raises, except the DCP pair, which degrades". The six parallel quotients are not live reads any more: they are a function of the configured leaves, computed at publish into bag leaves, answered override -> stamp -> published leaf. `dcp_enabled` now means "the launch configured DCP" (`dcp_size > 1`), not "a group is installed here". The two agree wherever `initialize_model_parallel` has stamped, and differ in a process that publishes without dist init, which is worth knowing before writing a test. `test_attn_dcp_defaults_when_group_is_uninitialized`, which the skill named as the pin, was replaced by config-shaped tests. - **the accessor shape to copy** -- `mamba_extra_buffer_enabled()`, `mamba_extra_buffer_lazy_enabled()`, `is_ep_joiner()`, `is_ep_scale_joiner()` are all gone. The shape to copy now is a `Derived(fn=...)` declared beside the leaves it is computed from, read as an ordinary bag leaf (`get_model().is_startup_weight_load_overlap`). - **the namespace-coverage ratchet** described in terms of the marker. Also states the consequence a test author actually trips over: overriding a leaf no longer moves its quotient, so a topology is stated by publishing a config or by naming the width. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Do not merge. This branch exists so the series can be read in one place. The
work is in five stacked pull requests, each of which is where review comments
belong:
GitHub shows a stacked PR only against its parent, so there is nowhere to see
the cumulative effect. The empty commit on top of #38113's branch is what makes
this a separate pull request from the same content.
What the series is
Configuration had two things living in one place.
ServerArgswas the record ofwhat the operator typed and the thing the rest of the process read its
settings out of. Those are different questions -- a field nobody set reads the
same as one set to the value resolution would have picked anyway -- and the
answer to the second one changes during startup while the answer to the first
must not.
They are separated into three surfaces:
None/Falsemeans "nottyped". Written once at construction, sealed for the length of resolution.
runtime code reads.
Everything in the series follows from that. The declarations move to
arg_groups/fields/, one class per namespace, so a namespace is one file andone class; the record is assembled from the input half of those classes, which
is what keeps a derived field like
attn_tp_sizeoff the wire format; theruntime's record reads become bag reads; and every value that is a pure function
of the configuration -- the predicates that existed once on each side of
publish, and the six parallel quotients -- becomes one declaration, computedonce at publish into the bag.
Numbers
server_args.pyNS(...)markers onServerArgsfieldsHow it was checked
Against
origin/main, at each step:namespace_ofmap, field by field: 494 / 494, identical;identical apart from the two ratio defaults this series deliberately changes;
sglang.srt.server_args: nothing lost;namespace;
19 failures on each side, the same 19, none of them config.
The last one matters more than it looks. An earlier, narrower sweep scoped to
"tests that set config themselves" reported clean while two files were red,
because the reach of a change like this is transitive --
test_kv_index_translatornever names
get_parallel(), it constructs an object that does.CI States
Latest PR Test (Base): ❌ Run #34014678339
Latest PR Test (Extra): ❌ Run #34014678242
Latest PR Test (AMD ROCm 7.2): ❌ Run #34014678365
CI States
Latest PR Test (Base): ❌ Run #34083722007
Latest PR Test (Extra): ❌ Run #34083721784
Latest PR Test (AMD ROCm 7.2): ❌ Run #34083722012