[CI vehicle, do not merge] config: round-3 ServerArgs series (#36789-#36792) - #36795
Closed
ch-wan wants to merge 16 commits into
Closed
[CI vehicle, do not merge] config: round-3 ServerArgs series (#36789-#36792)#36795ch-wan wants to merge 16 commits into
ch-wan wants to merge 16 commits into
Conversation
ch-wan
requested review from
BBuf,
Edwardf0t1,
Fridge003,
HaiShaw,
Ying1123,
alphabetc1,
hanming-lu,
hnyls2002,
huangtingwei9988,
hzh0425,
ispobock,
merrymercy,
xiezhq-hermann and
yizhang2077
as code owners
August 28, 2026 05:49
github-actions
Bot
added
lora
Multi-modal
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0c6acdc4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| max_world_size=max_world_size, | ||
| ) | ||
|
|
||
| get_parallel().stamp_derived_widths(**derived_widths) |
There was a problem hiding this comment.
When recovered_rank=True for an elastic EP scale joiner, initialize_model_parallel sets its local world_size to tensor_model_parallel_size * pipeline_model_parallel_size, while _init_parallel_groups initialized the actual WORLD group with ep_join_rank_offset + tp_size * pp_size. Stamping that local value here makes get_parallel().world_size permanently return the smaller joiner-local width instead of the live distributed world size, so consumers querying the topology after the join observe an incorrect rank count. Stamp torch.distributed.get_world_size() (or otherwise preserve the global size) for this case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a8d1340b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+289
to
+290
| def world_size(self) -> int: | ||
| return self._v("world_size", _ps().get_world_size) | ||
| return self._derived_width("world_size", _ps().get_world_size) |
There was a problem hiding this comment.
On an original elastic-EP worker, this now permanently prefers the world size stamped during launch, even after try_admit_scale_ranks() expands WORLD. _finalize_scale_up() calls update_dp_attention_post_scale(), but that function only restamps attn_dp_size, so get_parallel().world_size continues returning the pre-scale count instead of the expanded distributed size. Fresh evidence beyond the prior joiner-initialization issue is that the existing-rank scale-finalization path never updates or clears this stamp; refresh world_size when the scale commits or keep this property live.
Useful? React with 👍 / 👎.
ch-wan
force-pushed
the
cheng/gc-r3-ci
branch
5 times, most recently
from
August 28, 2026 07:16
d77a9b5 to
041fab3
Compare
Five handlers in `ServerArgs` decided nothing and only read a resolving view to raise: `_handle_kv4_compatibility`, `_handle_mxfp8_kv_cache_compatibility`, `_handle_prefill_only_disable_kv_cache`, `_handle_cache_compatibility` and `_handle_unified_memory_pool`. They now live in `arg_groups/kv_cache_hook.py`, and the record keeps a call at each slot, the shape `_handle_expert_pack` already had. The rule this series follows: a moved function keeps calling the record for anything the record still owns. `server_args._resolved_attention_backends()` stays a call on the record rather than its one-line body, because inlining it is production-identical and silently stops `patch.object(args, "_resolved_attention_backends")` from biting. Same for a sibling handler: the slot is the stable call point. The record loses 231 lines and gains no behaviour: 25 launch shapes that trip each check report the same exception type and message as before, and the 24-shape resolution dump is field-for-field identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight more handlers that decide nothing and only read a resolving view to raise or to set an environment variable. They join their families in `arg_groups/`: `parallel_hook.py` (context parallelism, DCP), `serving_hook.py` (SSL, ASR, multimodal config, crash-dump env) and `mamba_hook.py` (the Mamba backend and its int8 checkpoint rule). The record keeps a call at each slot. Two tests patched `is_cuda` / `is_sm100_supported` at `server_args`, which is no longer where the mamba rule reads them; they now patch `mamba_hook`. One of the two was green either way on this box, because the real answers happen to match there -- the patch had stopped doing anything. 62 launch shapes report the same outcome as before, and the 24-shape resolution dump is field-for-field identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ten handlers, one family: parsing `--cuda-graph-*` into a `CudaGraphConfig`, the compatibility rules that disable a capture backend, the DeepEP bucket alignment, and the two per-architecture prefill defaults. They live in `arg_groups/cuda_graph_hook.py` now and call each other directly; the record keeps a call at each slot. Moving them showed that two guards were reading the record's file and nothing else, so a handler that leaves stops being watched: - `test_model_config_reads_resolved_input` pins the opaque plugin callbacks against the first `get_model_config()` build. Its walk stopped at the slot, found a later build, and failed. It now follows the slot into `arg_groups/`. - `test_resolution_reads_no_bag` seeded its reachability walk from `server_args.py` and `overrides.py`. It now seeds from the whole package. 62 launch shapes report the same outcome as before, and the 24-shape resolution dump is field-for-field identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he record Sixteen more handlers, grouped by what they decide: `arg_groups/hicache_hook.py` (the hierarchical cache: DCP and storage-layout compatibility, host-memory mode, the ratio default), `platform_hook.py` (the NPU / MPS / AMD backend defaults and NCCL pre-warm), `moe_hook.py` (the MoE kernel rules), plus `handle_page_major_kv_layout` joining `kv_cache_hook.py` and four small ones joining `serving_hook.py` (media-URL security, the load balance method, the grammar backend, the debug utilities). `_handle_page_major_kv_layout` and the platform defaults declare, so this is the first batch where a moved handler writes to the stash: the slot passes the record and the function calls `declare_resolution` on it, the shape `expert_pack_hook` already used. 62 launch shapes report the same outcome as before, and the 24-shape resolution dump is field-for-field identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e record Six handlers, two families. `arg_groups/model_path_hook.py` resolves where the weights come from (the HF/GGUF reference, the ModelScope snapshot download, the object-storage URI). `arg_groups/attention_hook.py` holds the backend compatibility rules, the linear-attention backend selection, and the multi-item-scoring adjustment. 62 launch shapes report the same outcome as before, and the 24-shape resolution dump is field-for-field identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… out
Ten more handlers: data parallelism, DWDP, elastic EP, EPLB dispatch, the
legacy CP aliases and the expert-distribution metrics join `parallel_hook.py`;
the a2a-MoE backend rules join `moe_hook.py`; the XPU / CPU / HPU defaults join
`platform_hook.py`.
Three test couplings that a move breaks, two of them found by a census rather
than by a red test:
- `patch("sglang.srt.server_args.is_host_cpu_arm64")` no longer names anything;
the read went to `platform_hook`.
- Two `assertLogs` on the record's logger wrap a handler that now logs from
`parallel_hook`, so they saw nothing.
- `test/registered/unit/configs/test_multimodal_piecewise_cuda_graph.py` drives
`_apply_cuda_graph_compatibility`, which is why the seam rule in the first
commit of this series matters: it patches
`ServerArgs._disable_tc_piecewise_cudagraph_if_incompatible` on the class.
62 launch shapes report the same outcome as before, and the 24-shape resolution
dump is field-for-field identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seventeen handlers, the last of the bodies: `model_hook.py` (the per-model and per-capability adjustments, the mamba radix cache, the language-model-only rule), `memory_hook.py` (the GPU memory budget), `dllm_hook.py` (diffusion-LM inference), plus the deprecated-argument mapping, the environment variables, the remaining validations, the missing-value defaults, the hidden-states mode, the prefill delayer compatibility, tokenizer batching and the multimodal feature transport joining `serving_hook.py`; the load format joining `model_path_hook.py`; the encoder disaggregation joining `pd_disaggregation_hook.py`; and deterministic inference joining `attention_hook.py`. Four bodies stay, and none is a pipeline stage: `_resolved` and `_resolved_attention_backends` are the record's own read wrappers that moved handlers call back into, `_parsed_modelexpress_config` memoizes on the record, and `_apply_cuda_graph_disaggregation_roles` reads the record's lock set. `ServerArgs` is 6835 lines, from 11331: 483 field declarations, the CLI, the dispatcher, and a slot per handler. A slot keeps the method's own signature. `_handle_mamba_radix_cache(self, model_arch)` is the only handler that takes an argument, and a slot written as `(self)` would drop it. Two module-level names stay on the record and are read where they are used, so no hook module imports the record at module scope: `RADIX_SUPPORTED_DETERMINISTIC_ATTENTION_BACKEND` and `resolve_encoder_transfer_backend`. 62 launch shapes report the same outcome as before, and the 24-shape resolution dump is field-for-field identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four validators that decide nothing and only read a resolving view to raise: `_validate_deepep_v2_speculative_draft`, `_validate_deepep_v2_dispatch_token_budget`, `_validate_deepep_v2_model_architecture` and `_validate_cutedsl_a2a_token_budget`. They join the a2a rules they belong with in `arg_groups/moe_hook.py`. They were missed by the earlier passes because the census that drove those matched `_handle` / `_resolve` / `_parse` / `_apply` / `_disable` / `_set` and not `_validate`. Counting the class again with the prefix dropped: 68 method bodies remain, 2289 lines, and most of them are the record's read surface (`get_model_config`, `max_speculative_num_draft_tokens`, the batch-size generators) rather than pipeline stages. The budget check reads `max_prefill_buffer_tokens()` and `max_speculative_num_draft_tokens` off the record, and keeps doing so: they are the record's own derived properties. The slot keeps the name each one had, so the three test classes that call `args._validate_deepep_v2_*()` directly are untouched. 62 launch shapes and the 24-shape resolution dump are identical, and `server_args/` plus `test_runtime_context` pass (331). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fourteen checks that decide nothing and only read a resolving view to raise, placed with the family each belongs to: `lora_hook.py` (new) takes the LoRA argument checks and the LoRA/speculative compatibility rule; `kv_cache_hook`, `mamba_hook`, `cuda_graph_hook` and `model_path_hook` take theirs; and `validation_hook.py` (new) takes the ones that span no single family -- `check_server_args` itself, the bucket-rule parser, the load-publish arguments, the IB device list, the experimental marlin gate, the prefill/decode interval and the two-batch-overlap rule. `check_server_args` keeps its name on the record: it is a stage the launch path calls, like `resolve_once`, so the slot is the stage and the body is what moved. The slot forwards the return value now. `_validate_ib_devices` and `validate_transfer_engine` both return one, and a slot that dropped it would have handed every caller `None` -- a silent wrong answer rather than a crash. `is_cuda` stopped being imported by the record once the last of these left, so two tests that patched `sglang.srt.server_args.is_cuda` now patch the module the read went to. `server_args.py` is 6020 lines, from 6723. 62 launch shapes and the 24-shape resolution dump are identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_post_process_pass` refused the published record before running the pass,
so a pass that declares nothing could not run twice. `Engine.shutdown()` leaves
the launch published, and `Engine(server_args=sa)` with the same instance calls
`check_server_args()` again before republishing; `_hisparse_validation` reaches
the runner from there and returns `{}`, so the second launch raised.
The refusal now fires once there is something to record, which is what it was
for: a declaration made after publish is a silent no-op because the stash is
projected at publish and never again. A pass that declares nothing is a
validation, and validations are meant to run on every launch.
Reported by the codex reviewer on #36618. Reproduced: resolve, check, publish,
check again -> `ValueError: run_post_process_pass('_hisparse_validation')
called on the published config`.
`test_a_pass_that_declares_nothing_runs_on_the_published_record` pins both
directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… read back
`get_parallel().attn_tp_size` and its five siblings -- `attn_dp_size`,
`moe_ep_size`, `moe_tp_size`, `dcp_enabled`, `attn_dcp_size` -- were read back
off the group coordinators that had just been built from the
configured leaves. That made them the one corner of `get_parallel()` where a
name needs distributed init rather than a publish, and the two spellings are
indistinguishable at the call site: `tp_size` raises `ValueError("'parallel'
not published")`, `attn_tp_size` raised a group getter's bare `AssertionError`.
The arithmetic moves to `runtime_context.derive_parallel_widths`, and
`initialize_model_parallel` builds its groups from that dict and stamps it. One
formula, one place: the width a group was built at and the width a reader gets
cannot drift apart. `initialize_dp_attention` stamps `attn_dp_size`, and
`update_dp_attention_post_scale` restamps it where it already updates the live
one -- which is the case that makes a plain per-read derivation wrong. Elastic
EP rewrites `dp_size` and `ep_size` on the published bag while the coordinators
keep their construction width, so `tp_size == attn_tp_size * attn_dp_size *
attn_cp_size` stops holding after a scale-up; deriving on every read would
answer with a width that shrank, or zero.
`world_size` stays a live read and is deliberately not in the set. It is not a
quotient of the leaves, and its getter is right at every moment: a stamp taken
when the groups are built would answer with the launch count after
`try_admit_scale_ranks` expands WORLD, and with the joining cohort's own width
on a scale-joiner, which lays its groups out at `tp * pp` while WORLD spans
`ep_join_rank_offset + tp * pp`.
A scope that moves a width temporarily moves the derived one with it, the way
`patch_tensor_parallel_group` already does for `tp_size`: `disable_dp_size()`
runs a draft worker without DP attention, and overrides `attn_dp_size` for its
duration so the legacy getter and `get_parallel()` cannot disagree inside it.
A read is the override, then the stamp, then the live group. The fallback keeps
a process that installed groups without `initialize_model_parallel` working;
when neither is present the error says which of the two is missing.
`TestDerivedWidths` pins the quotients, the stamp winning over the group, the
override winning over the stamp, the restamp, the named failure, and that
`parallel_state` no longer carries its own copy of the arithmetic.
The 265 registered tests that mention the record report the same failure set as
the base commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…k helper `compute_dp_attention_world_info` recomputed `attn_dp_size` and `attn_tp_size` with the same two lines the stamped derivation uses. It cannot go away -- four of its six callers want only the ranks it returns, and ranks are per-process, so they are not part of the stamped set -- but the widths in it were a second copy that could drift from the one the groups are built at. `derive_attention_widths` is now the single home for that pair; `derive_parallel_widths` and the rank helper both call it. `initialize_dp_attention` had a third copy -- it called the rank helper, dropped the width it returns and recomputed `dp_size if enable_dp_attention else 1` three lines below -- and now takes what the helper already gave it. The guard that already refused a duplicate quotient in `parallel_state.py` covers `dp_attention.py` too, and refuses that `else 1` spelling as well, which the quotient patterns did not match. A new case checks the helper and the stamp agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`pool_configurator`, `mem_cache/registry` and `unified_radix_cache` read resolved configuration off a `ServerArgs` they were handed. The record holds the operator's raw input, so a resolution-filled field read there answers with the pre-resolution value; the published bag is what resolution decided. Eight reads move: `max_total_tokens` and `page_size` to `get_schedule()`, `enable_hisparse` / `radix_cache_backend` / `hicache_host_memory_mode` / `enable_session_radix_cache` to `get_memory()`, `enable_streaming_session` to `get_serving()`, `extra_metric_labels` to `get_observability()`. `max_speculative_num_draft_tokens` stays on the record: it is a derived property, not a field, so no bag serves it. These three are the first of the eight modules that the launch stand-in's write-through currently props up. Measured by deleting `_apply_fields(server_args, self._fields)` from `override_server_args` and running the 159 registered tests that mention the record: 27 failures across 11 files, of which 7 are the hook's own guards. Converting the readers is what lets that write-through -- and `_internal_write` with it -- go. `test_pool_configurator` read `page_size` off the record too, and now reads the same bag. 2171 tests across `mem_cache/`, `model_executor/` and `observability/` pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ctly Once a handler's body moved to `arg_groups/`, the method left behind was three lines of forwarding. Ninety-three of them are gone: the dispatcher, the other handlers and the tests call the hook function directly. Two kinds of slot stay. `check_server_args` is named by `entrypoints/engine.py`, so it is a stage the launch path calls rather than a forwarding artefact; and the five that forward into `arg_groups.overrides` (`_declare`, `_late_resolution`, ...) are the record's own declaration API. The argument mapping is read off each slot rather than assumed. A slot is not always `f(self, ...)`: `_validate_mamba_no_buffer(self, view, arch)` forwards `validate_mamba_no_buffer(view, arch)` and drops the receiver, and `_validate_mamba_extra_buffer` forwards one attribute of it rather than the record. Guessing either way produces a call that takes the wrong number of arguments. The slots were doing something besides forwarding, and it has to survive them: their import was **function-local**, which is what kept `server_args` importable at all. A hook module reaches `server_args` again through its own imports (`validation_hook` -> `kv_events` -> `server_args`), so hoisting those imports to module scope cycles. The record now imports each hook function at the top of the function that calls it. The record loses 403 lines and `arg_groups/` loses 31 of its 127 callbacks into the record; what is left there is the read surface (`get_model_config` and its kin) and the declaration API, with no pure forwarding among them. Three test seams move with the code: a `patch.object(ServerArgs, "_disable_...")` becomes a patch of the hook module, and two guards learn the dispatcher's second call spelling. Two completeness floors in `test_resolution_reads_the_declarations` were calibrated to the old class -- what the dispatcher still reaches inside the class is a handful of read wrappers now, and the package side is covered by `test_no_hook_reads_a_field_off_the_record`. 62 launch shapes and the 24-shape resolution dump are identical, and the 159 registered tests that mention the record report the same failure set as the base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing the forwarding slots removed their function-local import with them, and that import was load-bearing twice over. Hoisting them to module scope in `server_args.py` cycles (a hook reaches `server_args` again through its own imports), which the first version of this commit already worked around by importing at the top of each calling function. That is still too early: `_run_resolution_pipeline` returns for a dummy model after five steps, so importing all eighteen families at the top made `ServerArgs(model_path="dummy").resolve_once()` take 1.82s instead of 0.014s -- on the path every `override_server_args` in the test suite goes through. Each import now sits immediately before the first call it serves, and the same applies inside `arg_groups/`: a hook module that calls another imports it in the function that calls, not at module scope, so loading one family does not drag in a family it may never reach. A dummy resolution is back to 0.016s and 19 new modules, matching the parent commit, and loads six hook modules rather than eighteen. `TestResolutionStaysLazy` pins both halves: no module-level hook-to-hook import, and a dummy resolution that does not reach the model, cuda-graph or attention families. Reported by the codex reviewer on the slot cut, with the timing measured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Placeholder commit. This branch is one empty commit on top of PR D, so a run here exercises all four PRs of the series against main at once. It carries no change of its own and is not for merge. 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.
Motivation
This PR is the CI vehicle for a four-PR series, not a change. Do not merge it.
Its branch
cheng/gc-r3-ciis one empty commit on top of PR D, so a run here exercises allfour commits' worth of the series against
mainat once rather than four partial runs. Reviewcomments belong on the individual PRs.
Round 3 of the raw-input
ServerArgswork, following the six PRs of 2026-08-26(#36250–#36255) and the five of 2026-08-27 (#36618, #36725, #36620, #36621, #36622). Read them
in order:
server_args.py11327 → 6028)Together they finish moving the pipeline out of the record and close the last corner of
get_parallel()that answered a size by asking the group built from it.Modifications
None. One empty commit.
Accuracy Tests
Carried by the member PRs. Across the series, measured against the base commit:
server_args/ServerArgs/arg_groups, tip vsbase, with every failing file re-run alone: no regression in either direction.
ServerArgs(model_path="dummy").resolve_once()stays at 0.014s.Speed Tests and Profiling
None.
Checklist
Review and Merge Process
Close this once the four member PRs have landed. Do not merge it.
CI States
Latest PR Test (Base): 🚫 Run #33193697867
Latest PR Test (Extra): 🚫 Run #33193697989
Latest PR Test (AMD ROCm 7.2): 🚫 Run #33193698005