Skip to content

config: a parallel leaf with no live counterpart is read bare - #36620

Merged
ch-wan merged 1 commit into
mainfrom
cheng/gc-p3
Aug 27, 2026
Merged

ch-wan merged 1 commit into
mainfrom
cheng/gc-p3

Conversation

@ch-wan

@ch-wan ch-wan commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Motivation

PR 3 of a five-PR series on top of the raw-input ServerArgs work (#36250#36255), based on f775db03aaa. Each builds on the previous one; review them in order.

  1. cheng/gc-p1 — config: resolution declares, and nothing writes a field
  2. cheng/gc-p2 — config: every handler declares its cuda-graph decisions
  3. cheng/gc-p3 — config: a parallel leaf with no live counterpart is read bare ← this PR
  4. cheng/gc-p4 — config: a parallel size has one spelling; a patched scope declares its own
  5. cheng/gc-p5 — config: the record is not an object that gets passed around

They 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.

Two commits' worth of work, squashed: one enabling change in
runtime_context.py, and the sweep it makes possible.

get_parallel() served the live process groups; the resolved parallel
configuration was one hop away under .config. Of the 40 leaves in the
parallel namespace, 35 have no live counterpart at alldp_size,
ep_size, nnodes, enable_dp_attention and so on are config-only. For those
the hop disambiguates nothing: there is no live value they could be confused
with, and every reader paid for a distinction that does not exist.

Modifications

``d276ba7e211 · 125 files (118 production, 7 test) · **118 of them are one

The enabling change (read this one closely)

ParallelContext.__getattr__ was error-only: it raised, naming .config. It
now returns the leaf.

Three behaviours it has to keep, pinned in
test_launch_path_reads_configured_sizes.py (#36621 deletes that file once the
live tier is gone and re-homes these three into test_runtime_context.py):

  • a config-only leaf read bare equals what resolution decided (compared against
    resolution_result, not against the bag, so the two sides are different data
    paths);
  • before publish it raises ValueError("config namespace 'parallel' not published") — not a misleading AttributeError, which is what a naive
    fall-through gives. The leaf set is derived from the field metadata, since the
    bag is exactly what does not exist yet;
  • an unknown name still raises AttributeError.

Dynamo: gate helpers read parallel leaves inside compiled forwards, and
object.__getattribute__ graph-breaks. Measured before writing anything: bare
access through __getattr__ traces under torch.compile(fullgraph=True). There
is a regression test for the .config spelling already; the bare spelling is now
covered the same way.

The sweep

301 sites in 118 files: get_parallel().config.<leaf>get_parallel().<leaf>
— 296 spelled out in full, 5 read through a local alias (whose binding is
rewritten with them). Driven by AST — the receiver is resolved to
get_parallel() or a local bound to it, and only leaves in the config-only set
are rewritten. The five shadowed sizes are untouched here; #36621 collapses them.

One thing the sweep got wrong, and how it was caught

The rewrite turned an assertion in the launch-path guard into
bare == bare — a tautology, in the very test that was supposed to pin the
behaviour. It now compares against resolution_result. Worth knowing because
the same hazard applies to any mechanical sweep over a test that spells the
thing it is testing.

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: on
main it is a public non-field slot assigned in _handle_deprecated_args, and
this 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 read
parallel 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 a
regression test.

Checklist

Review and Merge Process

  1. Ping Merge Oncalls to start the process. See the PR Merge Process.
  2. Get approvals from CODEOWNERS and other reviewers.
  3. Trigger CI tests with comments or contact authorized users to do so.
    • Common commands include /tag-and-rerun-ci, /tag-run-ci-label, /rerun-failed-ci
  4. After green CI and required approvals, ask Merge Oncalls or people with Write permission to merge the PR.

🤖 Generated with Claude Code


CI States

Latest PR Test (Base): 🚫 Run #33110833966
Latest PR Test (Extra): 🚫 Run #33110833766
Latest PR Test (AMD ROCm 7.2): ❌ Run #33110833733

@ch-wan

ch-wan commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

@codex

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: dcdeba397a

ℹ️ 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".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ch-wan
ch-wan merged commit ca1d7ed into main Aug 27, 2026
19 of 24 checks passed
@ch-wan
ch-wan deleted the cheng/gc-p3 branch August 27, 2026 19:56
saturn-acc pushed a commit to saturn-acc/sglang that referenced this pull request Aug 31, 2026
nzr-niu pushed a commit to nzr-niu/sglang that referenced this pull request Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

amd deepseek hicache Hierarchical Caching for SGLang lora

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant