[Config] Round 6.5: a namespace declares what it derives, next to what it derives it from - #38113
Merged
Merged
Conversation
ch-wan
requested review from
BBuf,
CatherineSue,
Edwardf0t1,
Fridge003,
HaiShaw,
JustinTong0323,
Qiaolin-Yu,
Ying1123,
alphabetc1,
hanming-lu,
hebiao064,
hnyls2002,
huangtingwei9988,
hzh0425,
ispobock,
merrymercy,
slin1237,
xiezhq-hermann,
yeahdongcn,
yizhang2077 and
yuan-luo
as code owners
September 5, 2026 08:45
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 03:52
9e2f64f to
252445c
Compare
ch-wan
requested review from
alexnails,
fzyzcjy,
kpham-sgl,
liusy58 and
pyc96
as code owners
September 6, 2026 03:52
ch-wan
force-pushed
the
cheng/gc-r6-5-derived
branch
from
September 6, 2026 03:52
0e2fe1c to
06a485f
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-5-derived
branch
from
September 6, 2026 05:49
793c89a to
87271c9
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 05:56
252445c to
1684cbc
Compare
ch-wan
force-pushed
the
cheng/gc-r6-5-derived
branch
from
September 6, 2026 05:56
87271c9 to
b4854ac
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 07:12
1684cbc to
33f48ec
Compare
ch-wan
force-pushed
the
cheng/gc-r6-5-derived
branch
from
September 6, 2026 07:12
b4854ac to
1e2931a
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049, #38113 and #38194, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 08:43
33f48ec to
c23d063
Compare
ch-wan
force-pushed
the
cheng/gc-r6-5-derived
branch
from
September 6, 2026 08:43
1e2931a to
87e46e0
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 09:00
c23d063 to
c219bab
Compare
ch-wan
force-pushed
the
cheng/gc-r6-5-derived
branch
from
September 6, 2026 09:00
919098e to
2bce868
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 10:03
c219bab to
20ddbf0
Compare
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
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.
Fifth of five; stacked on #38049. The split gave every namespace a file, but
only for the half an operator types. This is the other half.
The parallel quotients are declared, not written out
attn_tp_sizeand its five siblings were sixty lines of near-identicalproperties in the runtime context, a file away from the leaves they are
quotients of, so reading
parallel.pytold you what you could set and nothingabout what that decides.
They are declared in
Parallelnow, in the same class as those leaves. Theycarry no annotation, so they are not dataclass fields and
collect_input_fieldsnever puts them on the record -- the same mechanism thatalready keeps
_NS_PATHoff it. That is the right exclusion: a quotient has nooperator input to preserve, and the record is what crosses a process boundary,
where a stamped width is one an elastic scale-up will not refresh.
A quotient is a value in the bag, like every other derived one
_derived_widthanswered from a stamp or, failing that, a live process group.The group read could never disagree with the stamp:
initialize_model_parallelstamps all six as its last statement,unconditionally;
attn_dp_sizethroughupdate_dp_attention_post_scale-- the comment claiming it does not waswrong;
multimodal_gen, which has its owninitialize_model_paralleland does notstamp, never reads a quotient.
So a built group was always already stamped, and the group read goes -- and with
it the last reason for a quotient to be resolved on every read.
Every input to
derive_parallel_widthsis a record field.dcp_enabledisdecode_context_parallel_size > 1, not a fact about a built group; it wasspelled
_DCP is not None, which is a longer way to say the same thing. So thesix are fixed once the configuration is fixed -- the same test every other
Derived(fn=...)in this PR passes. They are declared the same way and computedthe same way: once, at publish, into ordinary bag leaves.
What remains is override -> stamp -> published leaf. The stamp stays above the
leaf because an elastic scale-up restamps
attn_dp_size; the override stays ontop because that is how a test names a width.
One answer for the config-derived predicates
enable_mamba_extra_bufferand its lazy variant,is_ep_joiner,is_ep_scale_joiner,is_startup_weight_load_overlap: each existed as aServerArgsmember for the resolution pipeline and, for most of them, again asa
runtime_contextfunction for readers after publish. Three places to keepsaying the same thing.
A
Derived(fn=...)is a pure function of the published configuration, sopublishcomputes it once and stores it as an ordinary bag leaf -- a plainattribute 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 the mamba one does: it
reads
memory.disable_radix_cachealongside its ownexec.mambastrategy,which is why it could never have been a method on either bag.
The pre-publish helpers stay -- resolution needs the predicate before there is
a bag to read -- and three readers keep them, because they run before their own
process publishes:
initialize_dp_attention, which the weight-cache daemoncalls while building its groups thirty lines before its
publish, andPortArgs.init_new, a factory handed the record that already reads eighteenother fields off it.
Notes for a reviewer
Overriding a leaf does not move its quotient.
override(tp_size=2)leavesattn_tp_sizewhere the published config put it, because nothing is recomputedon read. A test states a topology by publishing a config -- which is what a
real process does -- or by naming the width it wants,
override(attn_tp_size=2).Six tests say it that way now. This is the price of having one answer computed
once, and it is the same price every other derived value in the config already
carries.
A caller that reads a quotient without publishing or overriding now gets an
explicit error naming the field, instead of a default that an uninitialised
group happened to supply. One fixture was in that state --
TestMlaWriteDoorsUnderDcpbuilt a bare pool and asked whether DCP was on --and it publishes a config now, which is what the process it stands in for
does.
Eighteen sites read these predicates without calling them. That is correct --
they are properties -- but it is worth saying they were checked, because a
census that assumes otherwise reports eighteen always-true conditions.
The skill that documents this subsystem is updated with it
.claude/rules/modify-component-must-read.mdpoints at.claude/skills/sglang-runtime-context/SKILL.mdbefore anyone touches thesefiles, so a stale sentence there is a wrong instruction rather than a stale
note. Four of its load-bearing statements stopped being true across this series
and are corrected here:
NS(...)is no longer how a field states its namespace(the declaring class is); the DCP degrade rule is gone, because the quotients
are not live reads;
mamba_extra_buffer_enabled()and the other predicatefunctions it named as the shape to copy no longer exist; and the
namespace-coverage ratchet is described in terms of the marker. The docstring of
test_server_args_namespaces.pysaid the same thing and is fixed too.The consequence a test author actually trips over is stated there as well:
overriding a leaf no longer moves its quotient, so a topology is stated by
publishing a config or by naming the width.
Verification
A full registered-unit sweep (648 files) against this stack's merge-base:
19 failures on both sides, the same 19 -- AMD
gfx950,modelopt,cuda_vmm,weight_checkerand friends, none of them config. The narrower 139-file config sweep used earlier in this seriesdoes not contain the files this change reaches --
test_kv_index_translatornever names
get_parallel(), it constructs an object that does -- which is whythe baseline differential over everything is what is quoted here.
CI States
Latest PR Test (Base): ❌ Run #34022763118
Latest PR Test (Extra): ❌ Run #34022763050
Latest PR Test (AMD ROCm 7.2): ❌ Run #34022763055
CI States
Latest PR Test (Base): ❌ Run #34084078137
Latest PR Test (Extra): ❌ Run #34084077964
Latest PR Test (AMD ROCm 7.2): ❌ Run #34084078108