[Config] Round 6.1: "unset" gets its own spelling, and the declaration says what it means - #38046
Merged
Merged
Conversation
ch-wan
force-pushed
the
cheng/gc-r6-1-pilots
branch
from
September 5, 2026 08:10
736e6c1 to
604ff16
Compare
ch-wan
force-pushed
the
cheng/gc-r6-1-pilots
branch
from
September 6, 2026 03:52
604ff16 to
ca635af
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
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
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-1-pilots
branch
from
September 6, 2026 07:12
ca635af to
188a027
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-1-pilots
branch
from
September 6, 2026 08:43
188a027 to
54b5114
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
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-1-pilots
branch
from
September 6, 2026 10:18
54b5114 to
6206a5e
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
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
added a commit
that referenced
this pull request
Sep 7, 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-1-pilots
branch
from
September 7, 2026 04:25
6206a5e to
0bf74d6
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>
ch-wan
force-pushed
the
cheng/gc-r6-1-pilots
branch
from
September 7, 2026 04:36
0bf74d6 to
320e3ae
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 7, 2026
…and the record is assembled from them (#38047) Second of four; stacked on #38046. Mechanical relocation plus one design change that the relocation makes possible. **Review by checking the identity proofs at the bottom** -- nothing here is meant to change behaviour. ## The declarations move `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. They move to `arg_groups/fields/`: one module per top-level namespace, 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 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. Two things travel with the declarations. The `*_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 all of them, because out-of-tree plugins 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. `server_args.py` goes from 4,458 lines to about 1,000. ## The record is assembled, not inherited Inheriting the namespace classes would make 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, since the record is what crosses a process boundary and a derived width pickled to a subprocess is a stamp that elastic scale-up will not refresh. `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. A namespace can now declare both halves side by side, and which half reaches the record is one readable call rather than an invariant spread across a base-class list. Nothing is registered on the derived side yet -- this is what makes it possible. `ServerArgs` is still one flat dataclass with 494 attributes, so `server_args.tp_size`, `ServerArgs(model_path=..., tp_size=8)`, pickling to a subprocess and every existing call site are untouched. ### Field order is a contract, so it is written down A dataclass turns field order into a positional constructor signature, and collecting whole namespaces groups fields that used to be interleaved. Keeping `model_path` first is not enough: `ServerArgs("dummy", "/tmp/tokenizer")` would set `load_format="/tmp/tokenizer"` and leave `tokenizer_path=None`, which then selects an invalid model loader -- silently, at a call site that did not change. So `arg_groups/field_order.py` records the order the record had before the split, and `collect_input_fields` orders what it collects by it. A field the record declares that the frozen order does not name goes after it, in declaration order -- the only backward-compatible place for a new field anyway, so a new declaration needs no edit there. The list is a compatibility record and nothing else reads it; the namespace a field belongs to is still the module it is declared in. ## Verification Four ways, all against the base commit: | check | result | |---|---| | `namespace_of` map, field by field | 494 / 494, **0 differences** | | CLI surface (options, defaults, choices, actions) | 507 / 507, **0 differences** | | field order, name by name | 494 / 494, **identical to the base** | | resolution result, 24 launch shapes x 489 fields | **0 differences** | | names importable from `sglang.srt.server_args` | nothing lost | Plus a full registered-unit sweep (648 files) against the stack's merge-base: 19 failures on both sides, the same 19, none of them config.
Shunkangz
added a commit
to Shunkangz/sglang
that referenced
this pull request
Sep 7, 2026
Conflict: python/sglang/srt/server_args.py main's "Config Round 6.x" refactor (sgl-project#38046-sgl-project#38113) moved every field declaration out of ServerArgs into per-namespace classes under arg_groups/fields/, shrinking server_args.py from 4455 to 1087 lines. The branch's only change to that file was the hicache_storage_key_scheme declaration, so the file takes main's version and the field moves to arg_groups/fields/memory.py, keeping its position after hicache_storage_prefetch_retry_max_attempts. NS("memory") is dropped: the declaring module is now the namespace. The field is deliberately NOT added to field_order.py. That record freezes ServerArgs' positional constructor signature; an unlisted name sorts after every listed one, which is the only backward-compatible slot for a new field. Consumers needed no change: hicache_hook.py already reads through resolving_view() and cache_controller.py through get_memory(), both of which are unaffected by where the declaration lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mqhc2020
pushed a commit
to mqhc2020/sglang
that referenced
this pull request
Sep 15, 2026
…n says what it means (sgl-project#38046) First of five. The stack continues a series that moved configuration out of `ServerArgs` and into the runtime context's namespace bags. This one fixes something that was actually broken, and gives the fix its other half. ## "Unset" gets its own spelling on two ratio fields `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`. `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. ## And the declaration says what the field means when nobody answers Making the default `None` leaves a hole: something has to supply the generic value. `Arg(fallback=...)` supplies it from the declaration. ```python 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". ### Which surface it lives on is the whole design Precedence becomes **override -> decision -> input -> fallback**, applied 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 two model families branch on exactly this: ```python # model_overrides/inkling.py, and the same shape in deepseek_v4.py if cfg.swa_full_tokens_ratio is None: overrides["swa_full_tokens_ratio"] = 0.1 ``` A fallback answering there is not "the generic value, later" -- a `__getattr__` layer is read-time, so there is no later. Every read during resolution would already get 0.8 and the branch would never fire. Running `_inkling_overrides` against both versions: ``` --- fallback on the effective surface only (this PR) --- cfg.swa_full_tokens_ratio during resolution = None family declared swa = 0.1 mamba = 0.1 --- fallback also on the view a pass reads --- cfg.swa_full_tokens_ratio during resolution = 0.8 family declared swa = None mamba = None <- the key never lands ``` 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. Exactly one reader consults the effective surface during resolution: the range check on the ratio, which wants the value the pools will be sized against. It asks `resolution_result` directly -- what its comment already claimed it was doing -- and it runs after the model families. ### The alternative, and why not A pass that fills the field in when nothing claimed it 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 (`"Unset means 0.8"`). An earlier revision of this series did exactly that and deleted it four PRs later. A declaration needs none of it, and `pipeline.py` is untouched by the whole series as a result. ### What may be declared this way, and what may not Across every hook, `if x is None: x = ...` appears at **55 sites over 29 fields**. They are not one thing: | | count | examples | declarable | |---|---|---|---| | unconditional constant | 5 | the two ratios, `grammar_backend="xgrammar"`, `mm_process_config={}`, `custom_weight_loader=[]` | **yes** | | unconditional, computed from another field | 4 | `tokenizer_path=model_path`, `device=get_device()`, `served_model_name`, `speculative_draft_model_quantization` | needs a `fallback="dotted.path"` form; not here | | **conditional decision** | ~20 | `chunked_prefill_size` across seven memory tiers, `max_bs` across eight, `max_running_requests` at 48 or 256 by model family | **no, and it should not be** | Only a value fixed for the life of the configuration belongs in a declaration. One that depends on the machine, on another field, or on anything impure (`random_seed = random.randint(...)`) is a decision, and decisions stay in a hook where their order is visible. This PR converts the two ratios only. ## Verification - `resolve_once` ends with the same effective values: the resolution result is identical across 24 launch shapes x 489 fields except for the two intended ratio changes. Separately, 16 launch shapes resolved on both sides, real model and dummy: 7,904 field readings, and the only difference is `random_seed`, a fresh `random.randint` per process. - The CLI registers the same 507 options with the same choices and actions; only the two defaults move. - `test_declared_fallbacks.py`, 17 cases. One pins the inverse of the dead branch above: what a pass sees while deciding is still `None`. - The whole series was swept over all 648 registered unit-test files against its merge-base: 19 failures on both sides, the same 19, none of them config. --- ### CI States Latest PR Test (Base): <!-- slot:pr-test:start -->:x: [Run #34083705463](https://github.com/sgl-project/sglang/actions/runs/34083705463)<!-- slot:pr-test:end --> Latest PR Test (Extra): <!-- slot:pr-test-extra:start -->:x: [Run #34083705284](https://github.com/sgl-project/sglang/actions/runs/34083705284)<!-- slot:pr-test-extra:end --> Latest PR Test (AMD ROCm 7.2): <!-- slot:pr-test-amd-rocm720:start -->:x: [Run #34083705383](https://github.com/sgl-project/sglang/actions/runs/34083705383)<!-- slot:pr-test-amd-rocm720:end --> <!-- pr-states:end -->
mqhc2020
pushed a commit
to mqhc2020/sglang
that referenced
this pull request
Sep 15, 2026
…and the record is assembled from them (sgl-project#38047) Second of four; stacked on sgl-project#38046. Mechanical relocation plus one design change that the relocation makes possible. **Review by checking the identity proofs at the bottom** -- nothing here is meant to change behaviour. ## The declarations move `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. They move to `arg_groups/fields/`: one module per top-level namespace, 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 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. Two things travel with the declarations. The `*_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 all of them, because out-of-tree plugins 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. `server_args.py` goes from 4,458 lines to about 1,000. ## The record is assembled, not inherited Inheriting the namespace classes would make 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, since the record is what crosses a process boundary and a derived width pickled to a subprocess is a stamp that elastic scale-up will not refresh. `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. A namespace can now declare both halves side by side, and which half reaches the record is one readable call rather than an invariant spread across a base-class list. Nothing is registered on the derived side yet -- this is what makes it possible. `ServerArgs` is still one flat dataclass with 494 attributes, so `server_args.tp_size`, `ServerArgs(model_path=..., tp_size=8)`, pickling to a subprocess and every existing call site are untouched. ### Field order is a contract, so it is written down A dataclass turns field order into a positional constructor signature, and collecting whole namespaces groups fields that used to be interleaved. Keeping `model_path` first is not enough: `ServerArgs("dummy", "/tmp/tokenizer")` would set `load_format="/tmp/tokenizer"` and leave `tokenizer_path=None`, which then selects an invalid model loader -- silently, at a call site that did not change. So `arg_groups/field_order.py` records the order the record had before the split, and `collect_input_fields` orders what it collects by it. A field the record declares that the frozen order does not name goes after it, in declaration order -- the only backward-compatible place for a new field anyway, so a new declaration needs no edit there. The list is a compatibility record and nothing else reads it; the namespace a field belongs to is still the module it is declared in. ## Verification Four ways, all against the base commit: | check | result | |---|---| | `namespace_of` map, field by field | 494 / 494, **0 differences** | | CLI surface (options, defaults, choices, actions) | 507 / 507, **0 differences** | | field order, name by name | 494 / 494, **identical to the base** | | resolution result, 24 launch shapes x 489 fields | **0 differences** | | names importable from `sglang.srt.server_args` | nothing lost | Plus a full registered-unit sweep (648 files) against the stack's merge-base: 19 failures on both sides, the same 19, none of them config.
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.
First of five. The stack continues a series that moved configuration out of
ServerArgsand into the runtime context's namespace bags. This one fixessomething that was actually broken, and gives the fix its other half.
"Unset" gets its own spelling on two ratio fields
swa_full_tokens_ratioandmamba_full_memory_ratiocarried real values astheir 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 typedand nothing else, and the family test becomes
is None.mamba_radix_cache_strategykeeps"auto": unlike the ratios it already has aspelling 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
ServerArgsany more.And the declaration says what the field means when nobody answers
Making the default
Noneleaves a hole: something has to supply the genericvalue.
Arg(fallback=...)supplies it from the declaration.The dataclass default stays
None. A fallback is not a default: the record isthe wire format, and a child process has to keep being able to tell "unset" from
"set to the value resolution would have picked anyway".
Which surface it lives on is the whole design
Precedence becomes override -> decision -> input -> fallback, applied in
resolution_result-- which the projection,/server_infoand every config bagread through.
Deliberately not in
resolving_view/resolved_view. Those are thedecision-over-input surface a pass reads while it is deciding, and two model
families branch on exactly this:
A fallback answering there is not "the generic value, later" -- a
__getattr__layer is read-time, so there is no later. Every read during resolution would
already get 0.8 and the branch would never fire. Running
_inkling_overridesagainst both versions:
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. Exactly one
reader consults the effective surface during resolution: the range check on the
ratio, which wants the value the pools will be sized against. It asks
resolution_resultdirectly -- what its comment already claimed it was doing --and it runs after the model families.
The alternative, and why not
A pass that fills the field in when nothing claimed it 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 (
"Unset means 0.8"). An earlier revision of this series did exactly thatand deleted it four PRs later. A declaration needs none of it, and
pipeline.pyis untouched by the whole series as a result.
What may be declared this way, and what may not
Across every hook,
if x is None: x = ...appears at 55 sites over 29fields. They are not one thing:
grammar_backend="xgrammar",mm_process_config={},custom_weight_loader=[]tokenizer_path=model_path,device=get_device(),served_model_name,speculative_draft_model_quantizationfallback="dotted.path"form; not herechunked_prefill_sizeacross seven memory tiers,max_bsacross eight,max_running_requestsat 48 or 256 by model familyOnly a value fixed for the life of the configuration belongs in a declaration.
One that depends on the machine, on another field, or on anything impure
(
random_seed = random.randint(...)) is a decision, and decisions stay in a hookwhere their order is visible. This PR converts the two ratios only.
Verification
resolve_onceends with the same effective values: the resolution result isidentical across 24 launch shapes x 489 fields except for the two intended
ratio changes. Separately, 16 launch shapes resolved on both sides, real model
and dummy: 7,904 field readings, and the only difference is
random_seed, afresh
random.randintper process.the two defaults move.
test_declared_fallbacks.py, 17 cases. One pins the inverse of the dead branchabove: what a pass sees while deciding is still
None.merge-base: 19 failures on both sides, the same 19, none of them config.
CI States
Latest PR Test (Base): ❌ Run #34083705463
Latest PR Test (Extra): ❌ Run #34083705284
Latest PR Test (AMD ROCm 7.2): ❌ Run #34083705383