[Config] Round 6.4: the runtime reads the bags, not the record - #38049
Merged
Merged
Conversation
ch-wan
requested review from
BBuf,
Edwardf0t1,
Fridge003,
HaiShaw,
JustinTong0323,
Qiaolin-Yu,
Ying1123,
alexnails,
alphabetc1,
fzyzcjy,
hanming-lu,
hnyls2002,
huangtingwei9988,
hzh0425,
ispobock,
jybsuper,
kpham-sgl,
lifuhuang,
liusy58,
merrymercy,
mickqian,
pyc96,
sufeng-buaa,
sundar24295s,
xiezhq-hermann,
yctseng0211,
yhyang201,
yizhang2077,
yuan-luo and
yushengsu-thu
as code owners
September 4, 2026 19:22
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 4, 2026 20:46
5504488 to
348377d
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 5, 2026 08:10
f39d589 to
7ae4820
Compare
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 5, 2026 08:10
348377d to
9e2f64f
Compare
This was referenced Sep 5, 2026
Merged
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 03:52
7ae4820 to
9582c70
Compare
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 03:52
9e2f64f to
252445c
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 05:56
252445c to
1684cbc
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 07:12
9582c70 to
e4772a5
Compare
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 07:12
1684cbc to
33f48ec
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049, #38113 and #38194, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 08:43
e4772a5 to
b35bd14
Compare
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 08:43
33f48ec to
c23d063
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 09:00
b35bd14 to
9a17063
Compare
ch-wan
force-pushed
the
cheng/gc-r6-4-readers
branch
from
September 6, 2026 09:00
c23d063 to
c219bab
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
The record is the operator's input; the bags are what is in effect. A reader that takes the record and reads a field off it gets the input, which is the wrong one of the two whenever resolution decided something -- and the mistake is silent, because for most fields and most launches the two agree. These seven already read both ways, sometimes in the same expression: `get_tokenizer(get_serving().tokenizer_path, tokenizer_mode=server_args.tokenizer_mode, ...)`. Every read here runs after its process publishes -- the two subprocess entry points publish before anything else, and the engine's own reads all sit below the launcher's publish -- so each one is a read of the same value from the surface that owns it. The parameters stay. Removing them is a signature change on call chains that reach constructors other implementations override, which is a separate decision from where a value is read.
Converting the reads left `_resolve_backend`, `_set_all_reduce_flags` and `_compute_parallelism_ranks` taking a record they no longer name. The dead- parameter ratchet is what noticed; the parameter and the argument go together at every call site. `_resolve_backend` shares its name with an unrelated function in `flashinfer_comm_fusion`, whose own callers and tests are untouched. Their callers keep theirs: `init_torch_distributed` still hands the record on.
Sixty-odd more files took the record and read config off it. Every one of them runs after its process publishes -- the launcher's own reads sit below `_launch_subprocesses`, the two subprocess entry points publish first thing, and the serving and model-executor layers only exist afterwards -- so each is the same value read from the surface that owns it. Two findings worth keeping. Eleven reads were `getattr(record, "field", default)`, which an AST scan for attribute access does not see: the census that said "43 readers" was counting the shape it could match, not the thing it was after. `incremental_streaming_output` was read that way twice, and the transcription tests were the only reason it surfaced. And not every record read is a bag read waiting to happen. A multimodal processor's `base_gpu_id` is the instance's, not the process's: two engines in one process keep different ones, and `test_publishing_another_config_does_not_move_the_device` exists to say so. That one stays on the record, while `rl_on_policy_target` beside it moves -- the test suite is what drew the line. The fixtures move with the code. Tests that hung config off a mock manager now publish a record, which is what the serving layer reads; where a test states a value, it says so with `override_server_args` instead of assigning through the mock.
… didn't The sweep caught what the file-scoped runs did not. Six functions were left holding a record they no longer name -- the ratchet names them -- and five test files drove code that now reads the bags without publishing anything, so the first read failed closed. Two reads go back to the record. `RequestMetricsExporter` is handed the directory it writes to at construction, and a test builds several with different ones; reading the process's value instead would make them the same exporter. That is the same line the multimodal processor's `base_gpu_id` sits on: a value one object owns is not the process's to answer for.
Open
2 tasks
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.
Last of four; stacked on #38048.
The record is the operator's input; the bags are what is in effect. A reader
that takes the record and reads a field off it gets the input, which is the
wrong one of the two whenever resolution decided something -- and the mistake is
silent, because for most fields and most launches the two agree. Several of
these files already read both ways, sometimes in the same expression:
Sixty-odd files convert. Record field reads in runtime code go from 199 to 11.
Nine parameters that the conversion emptied are dropped along with the argument
at every call site -- the dead-parameter ratchet is what names them.
"Runs after its process publishes" is a per-entry-point claim
Most converted reads sit in the serving and model-executor layers, which only
exist after publication, or in the two subprocess entry points, which publish
first thing. Three places are not like that, and they keep reading the record
they were handed:
HttpServerEngineAdapterlaunches the server as a child. The parentresolves the record and never publishes, so the adapter's own reads -- the
launch banner, the API key in its readiness loop, the TP width in
update_weights_from_tensor-- are ofself.server_args. A bag read herefails closed in a bare process, or answers for an unrelated engine in one that
happens to have published.
serve_grpcreads its sidecar port before the integrated servicer buildsthe
Enginethat publishes. The comment above that line already said so andalready bound
cfg = resolving_view(server_args)for it; the sidecar port andthe port it derives from read
cfg.initialize_dp_attentionruns from callers whose publish is notguaranteed, so its one predicate stays on the resolution view.
ROLE_NAMESPACE_SETS["dp_controller"]gainsobservabilityandserving,because the controller's metrics gate, tracing setup and worker-port broadcast
now read those namespaces. Under
SGLANG_ROLE_NAMESPACES=enforcethat set iswhat the process may read, so a conversion that reaches a new namespace has to
widen it in the same change.
Three things worth a reviewer's attention
Eleven reads were
getattr(record, "field", default). An AST scan forattribute access does not see those, so the census that said "43 readers" was
counting the shape it could match rather than the thing it was after.
incremental_streaming_outputwas read that way twice, and the transcriptiontests were the only reason it surfaced.
Not every record read is a bag read waiting to happen. A multimodal
processor's
base_gpu_idis the instance's, not the process's: two engines inone process keep different ones, and
test_publishing_another_config_does_not_move_the_deviceexists to say so. Itstays on the record while
rl_on_policy_targetbeside it moves.RequestMetricsExporteris the same shape -- it is handed the directory itwrites to, and a test builds several with different ones.
configure_loggerisa third: 17 call sites, one of which passes an
argparse.Namespace, so it isnot a global-context reader at all. Those eleven remaining reads are the ones
with a reason.
The fixtures move with the code. Tests that hung config off a mock manager
now publish a record, which is what the serving layer reads; where a test states
a value it says so with
override_server_argsinstead of assigning through themock.
test_hisparse_unitis the last of them: it stubbed aserver_argsontoa fake scheduler to say the decode radix cache was off, and the value it was
standing in for is the published default, so the stub goes and the class
publishes.
Two things CI caught that a local sweep could not
unittest.TestCase.enterContextis Python 3.11+. The converted fixtures usedit at 18 sites;
requires-pythonis>=3.10and CI runs 3.10, so every one ofthem raised
AttributeErrorthere while passing on a newer local interpreter.They call
enter_override(self, ...)now -- a four-line helper insglang/test/test_utils.pyover the override's owninstall()/restore().A batched sweep cannot see a missing publish. Three fixtures needed a
published config and did not have one; each passed inside a shard where some
other file had published, and failed when run alone. The affected cases are
test_serving_completions(which setincremental_streaming_outputon the mockmanager's record, where nothing reads it now),
test_qwen3_vl_feature_materialization(same shape for
mm_enable_dp_encoder), and the two Qwen Rust tests -- whosefixture already carried the comment
# Non-auto: get_resolved_model_impl would choke on a SimpleNamespacenext to themodel_implit sets, which is exactlywhat happened once
get_mm_processor_clsstarted reading that value from thebag. Its
publishmirrorsmodel_implnow, like the four fields it alreadymirrored.
Verification
A full registered-unit sweep (648 files) against this stack's merge-base:
19 failures on both sides, the same 19, none of them config. That sweep is what
caught 23 failures the file-scoped runs missed -- and, later, that the narrower
139-file list did not even contain the files this change reaches. It is also
what caught the
test_hisparse_unitfixture above: the file passes inside ashard where something else published, and fails when it is run on its own,
which is why every failing file is re-run alone before it is counted.
CI States
Latest PR Test (Base): ❌ Run #34015222310
Latest PR Test (Extra): ❌ Run #34015222199
Latest PR Test (AMD ROCm 7.2): ❌ Run #34015222269
CI States
Latest PR Test (Base): ❌ Run #34022670686
Latest PR Test (Extra): ❌ Run #34022670621
Latest PR Test (AMD ROCm 7.2): ❌ Run #34022670703
CI States
Latest PR Test (Base): ❌ Run #34027080180
Latest PR Test (Extra): ❌ Run #34027080040
Latest PR Test (AMD ROCm 7.2): ❌ Run #34027080191
CI States
Latest PR Test (Base): 🚫 Run #34084045998
Latest PR Test (Extra): 🚫 Run #34084045854
Latest PR Test (AMD ROCm 7.2): 🚫 Run #34084046012