config: a parallel size has one spelling; a patched scope declares its own - #36621
Conversation
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a4efe4398
ℹ️ 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".
| # tokenizer creates MmItemMemoryPool. A live attention subgroup | ||
| # size could leave acknowledgements missing and strand the lease. | ||
| ipc_consumer_count = max(get_parallel().config.tp_size, 1) | ||
| ipc_consumer_count = max(get_parallel().tp_size, 1) |
There was a problem hiding this comment.
Preserve the launch TP size for IPC acknowledgements
When Kimi image materialization runs inside patch_tensor_parallel_group with a smaller draft/attention TP group, that scope now overrides get_parallel().tp_size, so this passes the subgroup width instead of the tokenizer pool's full configured consumer count. In encoder-DP mode only the selected rank reconstructs each image, leaving the other acknowledgement slots unset and eventually preventing bounded IPC leases from being recycled. Preserve access to the launch TP width here; the same regression exists in kimi_k3.py.
Useful? React with 👍 / 👎.
…s own Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Motivation
PR 4 of a five-PR series on top of the raw-input
ServerArgswork (#36250–#36255), based onf775db03aaa. Each builds on the previous one; review them in order.cheng/gc-p1— config: resolution declares, and nothing writes a fieldcheng/gc-p2— config: every handler declares its cuda-graph decisionscheng/gc-p3— config: a parallel leaf with no live counterpart is read barecheng/gc-p4— config: a parallel size has one spelling; a patched scope declares its own ← this PRcheng/gc-p5— config: the record is not an object that gets passed aroundThey are grouped by how they have to be read, not by topic: PR 3 is 118 files of one mechanical rewrite, reviewed by checking the rule and sampling; PR 4 is the design change that rewrite made possible, and its production files each need reading.
CI for the whole series runs on a separate vehicle PR, whose branch sits one placeholder commit above PR 5: #36623.
After #36620,
get_parallel().configserved only five names — the ones that wereboth a live
ParallelContextproperty and aparallelconfig leaf:tp_size,pp_size,attn_cp_size,dcp_size,moe_dp_size. Bare meant "ask theprocess group",
.configmeant "what the launch asked for", and every readerhad to know which one it wanted.
This deletes the distinction.
get_parallel().tp_sizeanswers from thepublished configuration, and so do the other four.
get_parallel().configisgone, and so are the five live properties.
Modifications
``055e31a5711
· 62 files (48 production, 14 test) · +480 / −1327Why one spelling is enough
Because the groups are built at exactly the configured widths. Checked at every
assignment to the five module globals in
parallel_state.py:_TP,_PP— built fromtensor_model_parallel_size/ the stage count._ATTN_CP—_ATTN_CP = _TPonly underattn_cp_size == tp_size, where thewidths are equal by the condition; otherwise built with
attn_cp_sizeranks._DCP— groups ofdecode_context_parallel_size, which is the configureddcp_sizecarried through the scheduler's parallel snapshot._MOE_DP—_MOE_DP = _TPundermoe_dp_size == tp_size(equal again), and_MOE_DP = _ATTN_CPunderattn_cp_size > moe_dp_size. That last branch isthe one place a live width differs from the configured one.
And because exactly one place replaces a group after construction.
The scope that replaces a group declares it
patch_tensor_parallel_groupruns a scope under a different TP group — draftworkers of speculative decoding run the draft model at the target's attention-TP
width. It now overrides the three members that change:
A reader inside the scope gets the patched width; outside it gets the launch's.
draft_tp_contextgoes back to one line.A specific communicator's width is asked of that communicator
get_moe_cp_size()returns_get_moe_dp_group().world_size. That is the answerfor a reader that wants the group it will actually collectively operate on,
which is what the
_MOE_DP = _ATTN_CPbranch makes different from theconfiguration.
What the flip does to existing readers
Every previously-bare read of the five names now answers the configuration. On
origin/mainthose are 246tp_size, 51attn_cp_size, 21dcp_size, 6pp_sizeand 7moe_dp_sizesites (AST census, including theparallel = get_parallel()alias form). For the first four the value isidentical by the construction audit above. The seven
moe_dp_sizesites wereread one at a time:
qwen3_moe.py,mellum.pyassert attn_cp_size % moe_dp_size == 0x % x == 0under the alias; now compares real values. No launch changes: resolution already assertsmoe_dp_size == 1whenever the two differ, so it holds either way.qwen2_moe.pyself.moe_dp_sizeand never reads it.expert_pack_loader.pytp_size % attn_cp_size == 0, soattn_cp_size > 1impliestp_size > 1, and thetp_size != 1clause on the line above fires first.ipc_loader.pyget_moe_cp_size().attn_cp_sizeis not inCacheConfig, so collapsing this to the configured value lets two engines with different MoE sharding collide on one cache key.loader.py"dp"in the shard configget_moe_cp_size(). The same dict already carries the configured value under"moe_dp_size"; the live width was the point of the second entry.dumper.pyget_moe_cp_size(). It sits next tomoe_dp_rank, which is still live — a live rank beside a configured size misleads whoever is reading the dump.What else moved
__getattr__answers anyparallelleaf from the published bag and raises thesame
ValueError("... not published")before publish, so the 32 leaves do notneed 32 properties.
.config.<size>reads collapse to bare names.compute_world_size(config)becomescompute_world_size(*, enable_dp_attention, dp_size, tp_size, pp_size)— theRay driver calls it before any group exists.
compute_world_sizeis called with the four keywords, and theget_parallel()/get_exec()/get_context()stand-ins intest_scheduler_internal_state_world_size.pyand..._env_vars.pyare replaced by a published context(
override_server_args), which is what those tests were reaching for.sglang-runtime-contextskill is rewritten where it documented thedeleted API: the namespace table's parallel row, the dedicated
get_parallel()section, the "which accessor answers this question" entry fora size, the guard inventory's
_CONFIGURED_SIZE_CALL_SITESparagraph, and thedynamo note that called
__getattr__"error-only" (config: a parallel leaf with no live counterpart is read bare #36620 made it the read path,and measured that it traces under
fullgraph=True)._CONFIGURED_SIZE_CALL_SITES(48 entries) and its scanner leavetest_global_config_read_ratchet.py(−460 lines), andtest_launch_path_reads_configured_sizes.pyis deleted (−424). Both recordedwhich sites needed the config tier; there is no tier left to choose.
Accuracy Tests
No model-output change: this series moves where a configuration value is read
from, not what resolution decides. The equivalent check for that claim is a
resolution dump — every field's resolved value for 24 launch shapes (plain, tp2,
tp4_pp2, dp2, EAGLE, NEXTN, page32, page64_chunk2k, cuda-graph knobs,
disaggregation, deterministic, hierarchical cache, symmetric memory, …) — taken
in both trees and compared field by field:
0 differences across 24 shapes × 478 shared fields, against
f775db03aaa.The one field the series has and the base does not is
grpc_worker_threads: onmain it is a public non-field slot assigned in
_handle_deprecated_args, andthis series makes it a declared field. Its value is 4 on both sides.
Every guard also runs at each commit of the series, not only at the head — a
PR that is green only on top of its successors is not reviewable on its own. The
set is the config guards plus every registered test the series touches, ~33 files
per boundary, all green.
No GPU accuracy run. Everything above is CPU-side: resolution, projection and the
guards. A launch-path change that only shows up with real process groups is not
covered by any of it.
Speed Tests and Profiling
No benchmark run, and none is expected to move: nothing here changes a kernel, a
schedule, or the shape of any batch. What changes is the source of a
configuration read — a published dataclass attribute instead of a process-group
getter or an accessor hop.
The one place that could have mattered is
torch.compile: gate helpers readparallel leaves inside compiled forwards, and
object.__getattribute__graph-breaks. That was measured rather than assumed — the reads this series
introduces trace under
torch.compile(fullgraph=True), which is pinned by aregression test.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ci🤖 Generated with Claude Code
CI States
Latest PR Test (Base): 🚫 Run #33110887601
Latest PR Test (Extra): 🚫 Run #33110886999
Latest PR Test (AMD ROCm 7.2): ❌ Run #33110887336