config: every handler declares its cuda-graph decisions - #36619
Closed
ch-wan wants to merge 1 commit into
Closed
Conversation
ch-wan
requested review from
Fridge003,
Ying1123,
hnyls2002,
ispobock and
merrymercy
as code owners
August 27, 2026 05:07
Closed
5 tasks
Collaborator
Author
|
Codex Review: Didn't find any major issues. Nice work! 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". |
ch-wan
requested review from
iforgetmyname,
ping1jing2 and
whybeyoung
as code owners
August 27, 2026 07:56
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
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
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
PR 2 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 decisions ← this PRcheng/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 owncheng/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.
_parse_cuda_graph_configdeclares aCudaGraphConfig. Nineteen laterhandlers reached that declared object through
resolving_viewand edited it inplace, across 58 assignments:
The value ends up right — the stash holds the same object, so
resolution_resultand the bags answer correctly. What is wrong is theprovenance: the overrides log credits
_parse_cuda_graph_configfor decisions adozen other handlers make, and
validate_declarationsnever sees the laterchanges at all.
Modifications
``7e6b0c65f1f
· 5 files (3 production, 2 test) · +425 / −58, most of it inThe shape of the fix
with_phase(config, phase, **changes)incuda_graph_config.pyreturns a copywith one phase changed. It rebuilds both phases, so the result shares no
PhaseConfigwith the config it came from and an edit to either cannot reachthe other.
Each site becomes a declaration under its own handler's name:
_handle_gpu_memory_settingsis the exception: 22 of the 58 assignments areits, they are interdependent, and it reads its own earlier ones — so it works on
a
deepcopyand declares once at the end. The other 36, in 18 handlers, eachbecome a declaration.
Successive
with_phasedeclarations compose — the resolving view overlays thestash 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 ASTpass 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
TestDeclaredValuesAreNotEditedLaterwraps 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, andcompares 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"andenable_deterministic_inference=Trueeach reached a handler those three neverran, 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_configare['_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
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 #33110216870
Latest PR Test (Extra): ❌ Run #33110216704
Latest PR Test (AMD ROCm 7.2): ❌ Run #33110216788