config: resolution reads the declarations, not the fields - #36253
Merged
Conversation
github-actions
Bot
added
Multi-modal
ch-wan
marked this pull request as ready for review
August 25, 2026 03:18
ch-wan
requested review from
BBuf,
ClawSeven,
Edwardf0t1,
Fridge003,
HaiShaw,
JustinTong0323,
Qiaolin-Yu,
Ying1123,
alphabetc1,
hnyls2002,
iforgetmyname,
ispobock,
jybsuper,
lifuhuang,
merrymercy,
ping1jing2,
whybeyoung and
yushengsu-thu
as code owners
August 25, 2026 03:18
ch-wan
force-pushed
the
cheng/gc-p3-drop-unread-record
branch
from
August 25, 2026 03:47
5b7bdcf to
e744e0d
Compare
ch-wan
requested review from
Jiminator,
alexnails,
kpham-sgl,
liusy58,
mickqian and
pyc96
as code owners
August 25, 2026 03:47
ch-wan
requested review from
ByronHsu,
CatherineSue,
Duyi-Wang,
slin1237,
sogalin,
stmatengss and
xutizhou
as code owners
August 25, 2026 03:47
ch-wan
force-pushed
the
cheng/gc-p4-resolution-declarations
branch
2 times, most recently
from
August 26, 2026 05:39
faac6ba to
f17b3c4
Compare
ch-wan
force-pushed
the
cheng/gc-p3-drop-unread-record
branch
from
August 26, 2026 06:48
e744e0d to
c976856
Compare
ch-wan
force-pushed
the
cheng/gc-p4-resolution-declarations
branch
from
August 26, 2026 06:48
f17b3c4 to
f3f4dae
Compare
ch-wan
force-pushed
the
cheng/gc-p3-drop-unread-record
branch
from
August 26, 2026 07:22
c976856 to
3ca3f1b
Compare
ch-wan
force-pushed
the
cheng/gc-p4-resolution-declarations
branch
2 times, most recently
from
August 26, 2026 07:32
71043dd to
0eb2984
Compare
ch-wan
force-pushed
the
cheng/gc-p3-drop-unread-record
branch
from
August 26, 2026 08:12
3ca3f1b to
1725254
Compare
ch-wan
force-pushed
the
cheng/gc-p4-resolution-declarations
branch
from
August 26, 2026 08:12
0eb2984 to
6e6947d
Compare
ch-wan
force-pushed
the
cheng/gc-p3-drop-unread-record
branch
from
August 26, 2026 08:21
1725254 to
f63fd27
Compare
ch-wan
force-pushed
the
cheng/gc-p4-resolution-declarations
branch
2 times, most recently
from
August 26, 2026 08:59
444d916 to
4b1f9c3
Compare
ch-wan
force-pushed
the
cheng/gc-p3-drop-unread-record
branch
from
August 26, 2026 09:37
f63fd27 to
02afaaa
Compare
ch-wan
force-pushed
the
cheng/gc-p4-resolution-declarations
branch
from
August 26, 2026 09:37
4b1f9c3 to
e82e791
Compare
ch-wan
force-pushed
the
cheng/gc-p3-drop-unread-record
branch
from
August 26, 2026 11:52
02afaaa to
981bebc
Compare
`declare_resolution` writes the field as it declares, so a resolver that reads the field afterwards sees the declared value -- and every one of the 1130 mid-resolution field reads in this tree depends on that write. Removing it (the last step of making `ServerArgs` hold only raw input) currently breaks resolution outright: 20 of 20 launch shapes die on `AttributeError: 'NoneType' object has no attribute 'prefill'`. This is the first half of that: the readers move to `resolving_view`, a live view that answers from the declaration stash and falls through to the field. While the immediate write is still there the two agree, so this is a no-op -- which is exactly what makes it checkable: 20 launch shapes, every field of the resolution result compared against the previous commit, zero differences. 337 reads in `arg_groups`: the speculative hook (146), the override passes (103), the DeepSeek-V4 and PD-disaggregation hooks, and the three small model hooks. `ResolvedView` stays for the post-process passes, which want the state snapshotted at their slot; `ResolvingConfig` is for a reader that outlives a declaration.
…ields The other half of the readers: 779 `self.<field>` reads across the 88 resolution handlers reachable from the dispatcher now go through `resolving_view(self)`. Same argument as the hooks -- while `declare_resolution` still writes the field as it declares, the view and the field agree, so this changes nothing and can be checked exactly: 20 launch shapes, every field of the resolution result compared against the previous commit, zero differences. What it buys is that the pipeline no longer depends on that write to see its own decisions. Removing it -- so the record holds the raw input and nothing else -- needs the readers outside `arg_groups` and `ServerArgs` that resolution calls with the record (the platform defaults, the spec-algo hook, `ModelConfig`, the CP strategy) to move as well; those are next.
The last of the mid-resolution readers outside the pipeline's own modules: `ModelConfig.from_server_args` (23 reads), the spec-algo hook, the adaptive-spec support check, the CP strategy binder and the BCG predicate. They are called *by* resolution with the record in hand, so they have the same problem as the handlers -- a declaration-only resolver leaves the field holding the raw input. `resolving_view` is imported inside the function at these sites: they sit under `configs/`, `layers/` and `speculative/`, and a module-level import of `arg_groups.overrides` there would be a new import edge into the resolution pipeline. `test_model_config_reads_resolved_input` learns the spelling: a local bound to `resolving_view(sa)` / `resolved_view(sa)` / `sa._resolved()` is the record for scanning purposes, so its two pins keep describing the reads they were written for. 20 launch shapes, zero differences in the resolution result.
`ServerArgs`' public members read their fields, which is the same problem the handlers had: a declaration-only resolver leaves the field holding the raw input, so `max_speculative_num_draft_tokens`, `is_ep_joiner`, `is_startup_weight_load_overlap`, the expert-balancedness predicates and `describe_kv_events_publisher` could answer for what was typed instead of what resolution decided. They read through the view now, and the file settles on one spelling for it: `cfg = resolving_view(self)`, replacing the `resolved = resolved_view(self)` / `resolved = self._resolved()` mix this file had accumulated. One exception keeps its own name -- `describe_kv_events_publisher` already binds `cfg` to a `KVEventsConfig`, and the view has to not shadow it.
A census over the modules the pipeline actually reaches -- the registered passes and providers plus the import map, the same derivation `test_resolution_reads_no_bag` uses -- left 26 field reads outside `arg_groups` and `ServerArgs`: the dLLM config builder, the experimental Marlin LoRA validator, and the NPU platform defaults. The NPU one is the reason to bother. `set_default_server_args` asks "did anyone decide `page_size` yet?" before declaring its own default, and that question has to be asked of the declarations: reading the field would answer "no" for a size an earlier pass had already declared, and the hook would overwrite it. No A/B on a CUDA host can catch that, which is why the census is the check here. `configure_logger`'s single read stays: `log_level` is raw input, and that function is called with stand-ins.
Two reads in the dispatcher itself, one of which matters: `get_device_memory_capacity(self.device)` runs right after the platform defaults declare `device`, so a field read there would size memory for `auto`. The dummy-model boundary check moves with it for uniformity. The tests follow the same rule as the golden model-override ones: what they assert is what resolution decided, so they read `resolution_result` rather than the field -- the CPU-EAGLE overlap constraint, the dSpark draft-path default, the media-domain normalization, the multimodal piecewise-graph gates, the encoder transfer backend, and the spec-registry algorithm name. The multimodal processor fixture seeds the worker counts through `override_server_args` instead of a MagicMock, because the processor reads them from `get_mm()` now. All no-ops today (the declaration still writes the field as it declares); they are what the flip needs in place first.
Everything that will change hands when the declarations stop writing the field, moved ahead of the flip so it can be checked while both still agree. `test_server_args` is the bulk of it (101 reads): what those cases assert is what resolution decided, and `resolution_result` answers that whether or not the declaration was written back. Four assertions go the *other* way and now read the field on purpose -- the FA4 page-size and waterfill cases exist to show the field staying pristine while the declaration wins, so they keep reading it and say so. `_comparable` in the reproducibility suite reads the projection too: comparing fields would have stopped covering the decisions a resolution leak would shift. The multimodal and Kimi processor fixtures seed their worker counts and cache budget through `override_server_args` instead of a stand-in, because the processor reads them from `get_mm()` / `get_serving()` now -- that also fixes `test_kimi_processor_workers_clone_the_gpu_wrapper`, which the processor conversion broke (the cache came out enabled and the fingerprint path ran into a `SimpleNamespace` hf_config). The supplied-instance exposure pin drops 19 entries: the model-config, dLLM, CP, Marlin-LoRA, adaptive-spec and spec-registry reads all go through the view now.
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.
Motivation
Resolution is a chain: one resolver decides a field, the next one reads that decision.
Today that works only because
declare_resolutionwrites the field as a side effect, sothe record doubles as the scratchpad for a half-finished resolution. That side effect is
what keeps
ServerArgsfrom being what it should be — the raw user input — and it makes"who decided this value" unanswerable after the fact.
This PR moves every read that happens during resolution onto the declaration seam, so
the field write is no longer the channel the chain communicates through. It changes no
values: the view answers exactly what the field held.
What changes
resolving_view(server_args)is a live read view: every attribute read walks thedeclaration stash newest-first and falls through to the raw field. It is what code
running inside resolution should use. (
resolved_viewstays for post-resolutionpasses, which want a snapshot.)
arg_groupshooks, the ~779 field reads inside theServerArgsresolution handlers(
cfg = resolving_view(self)), the helpers resolution calls with the record, therecord's own member functions, and the type-based dispatcher all read through the view or
through
resolution_result(server_args, field).result rather than the field.
test_resolution_reads_the_declarations.pypins it: functions underarg_groups/*thattake a config, and every dispatcher-reachable
ServerArgshandler, must have zero directfield reads. That guard lands with the flip, so at this boundary the sweep is unpinned.
Verified by A/B: for every launch shape in the reproducibility suite, the resolved
projection before and after this PR is identical. Two launch shapes are deliberately not
identical, both because a pass declares without writing at this point in the stack, so a
read downstream of the pass's slot now sees the declaration instead of the pristine field.
Both restore what the pre-declaration monolith did, and neither is reachable from the CPU
suite:
_attention_backend_defaultdeclarestrtllm_mha, so theprefill-only no-KV path is no longer entered on a
Noneread._a2a_backend_overridesdeclaresdeepep, so theinvalid combination is now rejected at startup instead of admitted and then run on
deepep.
Three mid-resolution readers stay on the record here and convert with the flip in the last
PR of the stack —
get_attention_backends,describe_kv_events_publisher, andcompute_world_size. They are value-identical at this boundary.How to verify
🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #32966532779
Latest PR Test (Extra): ✅ Run #32966710903
Latest PR Test (AMD ROCm 7.2): ❌ Run #32966532866