config: the resolution pipeline's dispatcher leaves the record - #36896
Merged
Conversation
ch-wan
force-pushed
the
cheng/gc-r4a-pipeline
branch
from
August 28, 2026 22:11
847804a to
f8bf9a6
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ch-wan
marked this pull request as draft
August 28, 2026 23:25
ch-wan
force-pushed
the
cheng/gc-r4a-pipeline
branch
3 times, most recently
from
August 29, 2026 02:15
454bcb7 to
6c10b00
Compare
ch-wan
marked this pull request as ready for review
August 29, 2026 06:12
ch-wan
force-pushed
the
cheng/gc-r4a-pipeline
branch
2 times, most recently
from
August 29, 2026 06:35
92e4892 to
4c96ae3
Compare
This was referenced Aug 29, 2026
ch-wan
force-pushed
the
cheng/gc-r4a-pipeline
branch
from
August 29, 2026 07:14
4c96ae3 to
0f598b2
Compare
`_run_resolution_pipeline` was the last piece of resolution living on `ServerArgs`: 331 lines of ordered dispatch plus the four handler methods it still reached. Every step it calls already lives in `arg_groups/`, and none of them is a member of the record -- a step decides *about* the record. The dispatcher becomes `arg_groups.pipeline.run_resolution_pipeline`. Three of the four remaining handlers were one-line forwards to `run_post_process_pass`, like the ninety-three the previous PR cut, and are now that call; the fourth is the MLX opt-in check, which joins the platform family as `handle_hardware_runtime_validation`. `resolve_once` keeps the gate and calls the function. `ServerArgs` loses 366 lines (5643 -> 5277) and hosts no resolution handler at all. `test_no_handler_reads_a_field_off_self` said a handler on the record must not read a raw field off `self`; there is none left, so the guard becomes the stronger `test_the_record_hosts_no_resolution_handler`, and the package-side `test_no_hook_reads_a_field_off_the_record` covers what the steps read. Five guards follow the dispatcher to its new file: the two that count pipeline entries patch the module rather than the class, the "one caller" scan matches a bare-name call, the laziness and ModelConfig-ordering checks parse `arg_groups/pipeline.py`, and the platform write-capture plugin is installed in the namespace the capture now runs in -- without that last one the plugin's defaults silently stopped reaching the projection. The MLX check runs before the dummy short circuit, so `platform_hook` is imported at its call site like every other family. A dummy resolution loads two more modules (`pipeline`, `platform_hook`) and is unchanged at ~9ms. The 62-shape resolution probe reports byte-identical results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ch-wan
force-pushed
the
cheng/gc-r4a-pipeline
branch
from
August 29, 2026 08:11
0f598b2 to
829770e
Compare
kediwu0331
pushed a commit
to Zhylkaaa/sglang
that referenced
this pull request
Aug 30, 2026
…roject#36896) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
saturn-acc
pushed a commit
to saturn-acc/sglang
that referenced
this pull request
Aug 31, 2026
…roject#36896) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
9 tasks
5 tasks
nzr-niu
pushed a commit
to nzr-niu/sglang
that referenced
this pull request
Sep 1, 2026
…roject#36896) 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
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.
The series
Stacked, each PR based on the one above it. Read them in order; every boundary is
self-sufficient and green on its own.
main, not for mergeMotivation
_run_resolution_pipelinewas the last piece of resolution living onServerArgs: 331 lines ofordered dispatch, plus the four handler methods it still reached. Every step it calls already
lives in
arg_groups/, and none of them is a member of the record — a step decides about therecord, it is not part of it.
Keeping the dispatcher on the class also kept the awkward direction of travel: a hook in
arg_groups/was called by a method of the very object it was extracted from.Modifications
The dispatcher becomes
arg_groups.pipeline.run_resolution_pipeline(server_args).Three of the four remaining handlers were one-line forwards to
run_post_process_pass, the sameshape as the ninety-three slots #36792 cut, and are now that call. The fourth is the MLA opt-in
check; it joins the platform family as
handle_hardware_runtime_validation.resolve_oncekeepsthe gate — the resolved/failed flags — and calls the function.
ServerArgsgoes from 5643 to 5277 lines and hosts no resolution handler at all.That changes what one guard can say.
test_no_handler_reads_a_field_off_selfsaid a handler onthe record must not read a raw field off
self; there is no such handler left, so it becomes thestronger
test_the_record_hosts_no_resolution_handler, and the package-sidetest_no_hook_reads_a_field_off_the_recordcovers what the steps read.Five guards follow the dispatcher to its new file: the two that count pipeline entries patch the
module instead of the class; the "one caller" scan matches a bare-name call rather than a
self.attribute call; the laziness and ModelConfig-ordering checks parse
arg_groups/pipeline.py,which is the file whose imports map a bare-name call to the family defining it; and the platform
write-capture plugin is installed in the namespace the capture now runs in.
That last one is the seam worth noticing. The test installs an out-of-tree platform by patching
current_platformin the dispatcher's module. Moving the dispatcher moved that namespace, andwithout following it the plugin's defaults silently stopped reaching the projection — a real
behaviour the test exists to protect, failing loudly rather than quietly.
The MLA check runs before the dummy short circuit, so
platform_hookis imported at its call sitelike every other family rather than at module scope.
Accuracy Tests
The 62-shape resolution probe (
resolve_once()+check_server_args(), exception type and fullmessage) is byte-identical to the base commit. The registered tests that mention the record report
the same failure set as
main, file for file.A dummy resolution loads two more modules (
pipeline,platform_hook) and is unchanged at ~9 ms.Speed Tests and Profiling
None. The dispatch is the same sequence of calls, one stack frame shallower.
Checklist
Review and Merge Process
Mostly mechanical:
arg_groups/pipeline.pyis_run_resolution_pipelineverbatim withselfrenamed and four call sites rewritten. The parts worth reading are the guard changes in
test/,and in particular the platform-plugin patch target — the one place where moving the dispatcher
moves a namespace a test depends on.
Checked on this tip: the 77 handler and pass calls stay in the same order, the MLX validation still
runs immediately before the dummy return,
_run_resolution_pipelinehas no leftover caller, andthe plugin test patches
pipeline.current_platform.CI States
Latest PR Test (Base): ❌ Run #33242578573
Latest PR Test (Extra): ❌ Run #33242578477
Latest PR Test (AMD ROCm 7.2): ❌ Run #33242578540