config: the runtime readers take the published bags - #36254
Conversation
github-actions
Bot
added
documentation
fa26c5e to
e267df3
Compare
faac6ba to
f17b3c4
Compare
e267df3 to
dfdd18e
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfdd18e5fa
ℹ️ 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".
| if parsers.reasoning_parser == "auto" or parsers.tool_call_parser == "auto": | ||
| resolve_auto_parsers(server_args) | ||
|
|
||
| publish(server_args, role="tokenizer") |
There was a problem hiding this comment.
When SMG gRPC is launched with LoRA enabled, the integrated servicer subsequently enters Engine._launch_subprocesses, which calls check_server_args() again. The first check here has already normalized lora_paths into LoRARef objects, and this publish marks the same record as published; the second check then either rejects those objects as invalid path entries or attempts another _late_resolution, which explicitly refuses published configs. Consequently LoRA-enabled gRPC servers fail during startup; validation/parser resolution needs to happen only once, immediately before the engine's existing publish boundary.
Useful? React with 👍 / 👎.
f17b3c4 to
f3f4dae
Compare
c770839 to
2b10765
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b10765f8f
ℹ️ 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".
| server_args = server_args.replace_resolved( | ||
| "benchmark.one_batch", cuda_graph_config=explicit | ||
| ) |
There was a problem hiding this comment.
When a programmatic caller passes a ServerArgs that has already run resolve_once(), replace_resolved() preserves the resolved marker and merely appends this raw dictionary as the new declaration, so the following resolve_once() is a no-op. Publishing in latency_test() therefore installs a dict as get_exec().graph.cuda_graph_config, while model initialization immediately expects attributes such as .decode and .prefill, causing the one-batch benchmark to fail. Keep the replacement as a CudaGraphConfig for already-resolved records, or otherwise ensure the merged dictionary is parsed before publication.
Useful? React with 👍 / 👎.
f3f4dae to
71043dd
Compare
2b10765 to
33b799a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33b799a43b
ℹ️ 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".
| # JSON (resolution applies that last), so the size is merged into that JSON. | ||
| # It travels as a copy: a field assignment is the shape the mutation ratchet | ||
| # pins at zero, and this record is unresolved, so the copy is a plain one. | ||
| if getattr(server_args, "_resolution_finished", False): |
There was a problem hiding this comment.
When a programmatic caller has already called resolve_once(), this branch still never runs because ServerArgs records completion in _declarations_materialized, not _resolution_finished (the latter appears nowhere else in the repository). The fallback consequently supplies a plain dict to replace_resolved() while preserving the resolved state, so the subsequent resolve_once() is a no-op and publication exposes an untyped CUDA graph config to model initialization. Fresh evidence beyond the existing comment is that the attempted fix checks a nonexistent marker; use the real marker or otherwise preserve a CudaGraphConfig for resolved inputs.
Useful? React with 👍 / 👎.
71043dd to
0eb2984
Compare
40bbed0 to
de7d5ac
Compare
`resolve_image_processor_backend` already had one caller passing `get_mm()` and three passing a record; all four pass the bag now, and the parameter is named for what it is. The FlashInfer all-reduce fusion resolver and the draft attention-backend fallback read their leaves directly and take no config at all -- the fusion one was reaching for `get_server_args()`, which is a global record read that only escaped the ratchet because it handed the whole object to a helper. `reserve_rope_cache_for_long_sequences` reads `model.context_length` and the two `spec` counts. The FlashInfer fusion test drives `_resolve_backend(backend, is_multi_node)` directly: the arch dispatch is what those cases are about, and the entry above it now takes no arguments. Five (file, field) pairs leave the supplied-instance exposure pin.
`init_cp_strategy(server_args)` was called from two places that cannot read the same source: resolution calls it inside `__post_init__`, where the bags do not exist yet, and `get_cp_strategy` calls it lazily in a worker process, where the record is not where the resolved sizes live -- that path was reaching for `get_server_args()` and handing the whole object over, which is how a global record read escapes the ratchet. It takes `enable_prefill_cp`, `cp_size` and `cp_strategy` now. Resolution passes them off its view, the lazy path off `get_parallel().config`, and the unit tests pass them directly instead of building a `SimpleNamespace` per case.
`resolved_embedding_plan` is the `/server_info` and gRPC readback of the embedding runtime knobs, and it read `cuda_graph_config`, `chunked_prefill_size`, `disable_radix_cache`, `is_embedding` and `prefill_only_disable_kv_cache` off the record -- which now holds the raw input, so the plan would have reported `None` for the graph config of a server running one. The two callers pass `resolving_view(record)`, and the parameter is `config` rather than `server_args`: the function's contract is "something that answers with the resolved configuration", which is why it was duck-typed to begin with. Five (file, field) pairs leave the exposure pin, which is down to four -- the launcher's pre-publish env setup and the auto-parser late resolution, both of which read the record because that is the only thing that exists at those points.
All four read the record because the record is the only thing that exists where they run: the NCCL environment setup and the auto-parser late resolution both happen in the launcher before the publish. Written down next to the pin so the next person does not have to re-derive it, and so a new entry has to come with the same kind of reason.
The biggest remaining clusters of "read a field off a handed record", all of
them past their process entry's publish:
* the encoder's five modules read `get_parallel().config.tp_size` and
`get_serving().host` / `.port` -- `runtime.py` was already mixing
`get_parallel().config.dp_size` with `server_args.tp_size` on one line;
* `get_allocator_type()` reads the two HiCache leaves and takes no config,
which drops the parameter from `_get_allocator_type` and its 14 call sites
in the hybrid pool assembler;
* the unified radix cache's write and prefetch policies, the detokenizer and
gRPC metrics flags, the XPU and runner-backend memory-saver checks, the
CP DSA split, the FP4 GEMM backend and the EP redundant-expert count.
Both exposure pins are now at their floor: four entries in `_EXPOSED` and three
in `_OVERRIDDEN_AND_READ`, all of them the launcher's pre-publish env setup and
the auto-parser late resolution.
`_setup_and_run_http_server` runs after `Engine._launch_subprocesses` has
published, so its host, port, log level and metrics flag come from
`get_serving()` / `get_observability()` -- 28 reads, including the two
`enable_metrics` gates in the app setup.
The DP controller keeps its record reads: its declared namespace set is
`{exec, parallel, device, disagg}`, so reading `serving` or `observability`
there would be refused under `SGLANG_ROLE_NAMESPACES=enforce`. Narrowing that
set was the point of declaring it, and widening it to move a `host` read is the
wrong trade.
Six sites, split by what they are actually asking. The dual-chunk attention backend shards over the *live* group, so it reads `get_parallel().tp_size` like the rest of the head-count arithmetic in the tree. The runner windows, the `/v1/loads` accelerator count, the NIXL rank arithmetic and the tokenizer's worker division all want the launch width in a process that holds no model groups, so they read `get_parallel().config.tp_size` and are registered with that reason.
`RayEngine` publishes as part of `Engine._launch_subprocesses` and lays the actors out afterwards, so all 22 record reads in the two driver modules were reading the raw input where the bag was already available -- and both files were already mixing the two, `_compute_world_size` multiplying `get_parallel().config.pp_size` by `server_args.tp_size` on one line. A launch that leaves `dp_size` to resolution would have sized the placement group from `None`. Both modules are at zero record reads now, with a local `parallel = get_parallel().config` where a function reads several. `_compute_world_size` takes no argument. The four new configured-size reads are registered with their reason: the driver is sizing the actors that will hold the process groups, so there is nothing live to ask. The Ray path has no CI coverage (`test/manual/test_ray_engine.py` boots a real cluster), so `test_ray_driver_reads_the_bags` pins it three ways: the world-size arithmetic against a published config, the same arithmetic following a post-publish `override` -- which is what separates a bag read from a record read -- and a file-scoped check that neither module reads a field off an instance. It reports all 22 reads on the pre-conversion tree. Verified against a real cluster with a cached model: `TestRayEngineOfflineTP1` and `TestRayEngineOfflineTP2` pass (5 tests), and the custom-placement-group case launches, serves and shuts down -- it dies afterwards on a Ray GCS teardown timeout that `origin/main` hits identically.
Nine reads in eight files, each already mixing bag reads with a record read: the KV configurator's memory-saver flag, the dist-init host in `bootstrap`, the gRPC and sidecar hosts, the rust server's transport width and hosts, the in-process HTTP engine adapter, and the MindSpore runner's host. `initialize_moe_config` and `initialize_fp4_gemm_config` go with them, and they change signature: both were handed a record they read resolution's answers off (`moe_a2a_backend`, `deepep_mode`, `quantization`, the speculative pair) or, in the fp4 case, no longer read at all. They take no argument now and read `exec.moe` / `spec` / `model` / `exec.kernel`, which every caller has published by the time it calls -- the scheduler, the weight-cache daemon, and the `one_batch` work function. The five `layers/moe/utils.py` exposure pins go with the conversion, and the three tests that used to hand it a stand-in publish one. `configure_logger` keeps its record read, and this is the reason: it runs before the publish in the launcher and in the encoder HTTP entry, it is called with stand-ins, and `multimodal_gen` calls it with a *different* `ServerArgs` class that has no bags at all. A bag read there would raise on three separate paths. What is left of the 172 resolution-named instance reads this clearing started from is 39, in five places, all structural: the launcher before its publish (19), the DP controller whose declared namespace set excludes `serving` and `observability` (14), the auto-parser late resolution (4), the multimodal processor's per-instance `base_gpu_id`/`tp_size` (engines sharing a process each have their own), and `configure_logger`.
1 participant
Motivation
These are the readers that run after the config is published and still read the record.
Each one is a place where a value the user did not set — one that resolution decided — is
read from the wrong side. Two of them were user-visible:
get_parallel().config.pp_size * server_args.tp_size. A launch that lets resolutiondecide
dp_sizecomputed the group size fromNone./server_inforeportedcuda_graph_config = Nonefor a serverthat was in fact running one, because the plan read the record instead of the resolved
configuration.
What changes
Readers move to the published bags: the CP strategy binder now takes the three values it
needs (
enable_prefill_cp,cp_size,cp_strategy) rather than a record; the embeddingplan reports the resolved configuration; the encoder, HiCache and metrics readers, the HTTP
entry (
servingandobservability), the lasttp_sizereaders, and all 22 reads in theRay driver (
ray/engine.py,ray/data_parallel_controller.py) take the bags._compute_world_size()no longer takes an argument.Two process-init seeders change signature with them.
initialize_moe_configwas handed arecord it read resolution's answers off (
moe_a2a_backend,deepep_mode,quantization,the speculative pair); it takes no argument now and reads
exec.moe/spec/model,which every caller has published by the time it calls — the scheduler, the weight-cache
daemon and the
one_batchwork function.initialize_fp4_gemm_confighad already stoppedreading its
server_argsparameter and drops it.No supplied-instance read stays: the encoder's
LoadConfigwas the last one, and theexposure ratchet's pin set is empty as of this PR, so the next such read is a new entry
that has to argue for itself.
configure_loggeralso keeps itsrecord read: it runs before publish in the launcher and in the encoder HTTP entry, it is
called with stand-ins, and the
multimodal_gentree calls it with a different, same-namedServerArgsclass that has no bags.New coverage:
test_ray_driver_reads_the_bags.pypins the world-size arithmetic against apublished config, checks that the driver follows a post-publish override (which is what
distinguishes a bag read from a record read), and asserts the two Ray files contain zero
record reads — on the pre-conversion tree that assertion reports all 22.
How to verify
The Ray path itself has no CI coverage (
test/manual/test_ray_engine.pystarts a realcluster). It was run against a real cluster with a cached model:
TestRayEngineOfflineTP1+TestRayEngineOfflineTP2= 5 passed;TestRayEnginePlacementGroupserves and shuts down, then the process dies in Ray's GCSteardown timeout —
origin/maindies the same way.🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #32966792220
Latest PR Test (Extra): ✅ Run #32966967988
Latest PR Test (AMD ROCm 7.2): ❌ Run #32966792281