Skip to content

config: every handler declares its cuda-graph decisions - #36725

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

config: every handler declares its cuda-graph decisions#36725
ch-wan merged 1 commit into
mainfrom
cheng/gc-p2

Conversation

@ch-wan

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

Copy link
Copy Markdown
Collaborator

Motivation

PR 2 of a five-PR series on top of the raw-input ServerArgs work (#36250#36255), based on d1f14431fdf. 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 ← this PR
  3. cheng/gc-p3 — config: a parallel leaf with no live counterpart is read bare
  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.

_parse_cuda_graph_config declares a CudaGraphConfig. Nineteen later
handlers reached that declared object through resolving_view and edited it in
place, across 58 assignments:

cfg = resolving_view(self)
cfg.cuda_graph_config.decode.max_bs = 8

The value ends up right — the stash holds the same object, so
resolution_result and the bags answer correctly. What is wrong is the
provenance: the overrides log credits _parse_cuda_graph_config for decisions a
dozen other handlers make, and validate_declarations never sees the later
changes at all.

Modifications

``7e6b0c65f1f · 5 files (3 production, 2 test) · +425 / −58, most of it in

The shape of the fix

with_phase(config, phase, **changes) in cuda_graph_config.py returns a copy
with one phase changed. It rebuilds both phases, so the result shares no
PhaseConfig with the config it came from and an edit to either cannot reach
the other.

Each site becomes a declaration under its own handler's name:

self._declare(
    "_disable_prefill_cuda_graph_for_deepseek_trtllm_mla",
    cuda_graph_config=with_phase(
        cfg.cuda_graph_config, Phase.PREFILL, backend=Backend.DISABLED
    ),
)

_handle_gpu_memory_settings is the exception: 22 of the 58 assignments are
its, they are interdependent, and it reads its own earlier ones — so it works on
a deepcopy and declares once at the end. The other 36, in 18 handlers, each
become a declaration.

Successive with_phase declarations compose — the resolving view overlays the
stash live, verified directly before converting anything.

Counting

The first census said "20 sites in one method". That was a narrow grep: it
matched only the local-alias spelling (decode_cuda_graph_config.max_bs = ...)
and missed the direct one (cfg.cuda_graph_config.decode.backend = ...). An AST
pass over the class gives the real surface, 58 assignments in 19 methods. The
grep also matched == comparisons until I added a negative lookahead —
converting one of those into a declaration would have been a real bug.

The guard

TestDeclaredValuesAreNotEditedLater wraps all four stash-append channels
(declare_resolution, declare_late_resolution, declare_direct_writes,
run_post_process_pass), deep-copies each entry the moment it lands, and
compares at the end. Eight launch shapes.

Widening that matrix is what turned the guard into a probe. With the first three
shapes it passed; disaggregation_mode="prefill" and
enable_deterministic_inference=True each reached a handler those three never
ran, and each was still drifting. The DP-attention shape then caught two more —
handlers that reach the config through a local alias, which the first pass of
the conversion had missed.

Evidence

Before: the sources for cuda_graph_config are ['_parse_cuda_graph_config']
on every shape. After: ['_handle_gpu_memory_settings', '_parse_cuda_graph_config']
and the per-handler entries. The resolved value is identical on all 24 shapes.

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

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 #33110783844
Latest PR Test (Extra): 🚫 Run #33110783520
Latest PR Test (AMD ROCm 7.2): ⏳ Run #33110783941

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ch-wan
ch-wan merged commit 7c3b5a6 into main Aug 27, 2026
16 of 23 checks passed
@ch-wan
ch-wan deleted the cheng/gc-p2 branch August 27, 2026 19:55
saturn-acc pushed a commit to saturn-acc/sglang that referenced this pull request Aug 31, 2026
…36725)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
nzr-niu pushed a commit to nzr-niu/sglang that referenced this pull request Sep 1, 2026
…36725)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant