config: the readback and the resolving view say what they are - #35027
Conversation
![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: d4fab67974
ℹ️ 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".
| result: Dict[str, Any] = dataclasses.asdict( | ||
| self.tokenizer_manager.server_args | ||
| ) |
There was a problem hiding this comment.
When the gRPC server is launched with either parser set to auto, replacing the resolved overlay here makes GetServerInfo return the literal startup value auto. The HTTP /model_info handler now compensates by returning both effective parser values, but RuntimeHandle.get_model_info() still omits them, so gRPC clients have no API that reports which parsers are actually running. Add the resolved parser fields to the gRPC model-info JSON before dropping them from this response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 14b4010: the gRPC model-info response now carries the resolved reasoning_parser and tool_call_parser. test/registered/unit/entrypoints/test_effective_state_surfaces.py pins HTTP, gRPC, and Engine to report the same effective fields so one surface can't silently lag.
5e8bb83 to
8a626d9
Compare
965b88a to
2402291
Compare
fbfb54f to
9b12646
Compare
ch-wan
left a comment
ch-wan
left a comment
There was a problem hiding this comment.
Summary
This increment makes /server_info and the gRPC/Engine twins report the ServerArgs record (dataclasses.asdict), moves post-publish facts onto /model_info (HTTP, gRPC, new Engine.get_model_info), and teaches the rust HTTP replacement the same keys from the launch blob. HTTP/gRPC/Engine live fields (weight_version, load_format, reasoning_parser, tool_call_parser) all go through config_value, and the new AST ratchet pins that source, not just key names. Prior items: the 404 /get_weight_version test comment is gone; the /server_info FastAPI docstring is a visible migration note; leftover overlay wording still exists outside this diff (TokenizerManager.record_config_updates, RuntimeContext.resolved_server_args_dict, test_tokenizer_config_updates.py).
Issue counts by severity
- bugs: 0
- suggestions: 1
- nits: 0
| log the production writers filled. | ||
| """ | ||
|
|
||
| async def _fake_internal_state(): |
There was a problem hiding this comment.
[suggestion] _call_server_info_with now documents that config_updates go through record_config_updates "so the endpoint answers a log the production writers filled." That is the old overlay contract this change dropped: the only caller that passes config_updates is test_the_readback_reports_the_record_not_the_control_plane, which asserts /server_info still returns the record (v1) after the log records v2. The new sentence re-teaches the leftover overlay story the last review asked to remove.
Suggestion: Say the helper records the update through the production writer so the test can prove /server_info ignores that log; the overlay is resolved_config_dict / crash dump / /model_info, not this endpoint.
2402291 to
d2c10c8
Compare
9b12646 to
9d350e6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d350e6d0c
ℹ️ 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".
| **_global_state.tokenizer_manager.resolved_config_dict( | ||
| dataclasses.asdict(server_args) | ||
| ), | ||
| **dataclasses.asdict(server_args), |
There was a problem hiding this comment.
When update_weights_from_disk succeeds, _update_model_path_info() changes the manager-owned model_path and served_model_name, but this now returns their launch-time values. This breaks worker discovery after such an update: experimental/sgl-router/src/workers/introspect.rs obtains served_model_name exclusively from /server_info and uses it to populate the worker's routed model IDs, while the HTTP /model_info response does not include served_model_name. A router that discovers or re-registers the worker after the swap therefore registers the old model ID and cannot route requests for the newly served model; retain the effective overlay for these identity fields or move the field and update the discovery consumer together.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by moving the field and its consumer together, as you suggested, rather than keeping the overlay.
served_model_name now appears on all three Python model-info surfaces (HTTP, gRPC, Engine) and on the rust server's /model_info. experimental/sgl-router/src/workers/introspect.rs reads it from /model_info, falling back to /server_info for workers that predate the field there, so a mixed-version fleet keeps working.
Two notes on the router change. The requests run concurrently, so worker discovery does not get slower for the extra endpoint; and they fail independently — a worker that answers only one still registers with whatever did answer, where before a single failed round-trip lost everything. Four new tests cover it: prefers /model_info, falls back when the field is absent there, treats an empty name as absent on either surface, and keeps the name when /server_info is unreachable.
The parity guard also gained an assertion that every model-info surface reports the manager-owned identity. Those two fields are subtracted from the writer-derived required set because a weight update moves them on the manager rather than through override; that exemption now has a check behind it instead of a claim, and the claim was false when it was written — only Engine carried served_model_name.
ch-wan
left a comment
ch-wan
left a comment
There was a problem hiding this comment.
Summary
This increment does the split it claims: HTTP/gRPC/Engine /server_info serialize the ServerArgs record, the crash dump still goes through resolved_config_dict, and live weight_version / load_format / parsers are read via config_value on HTTP /model_info, gRPC get_model_info, and the new Engine.get_model_info. The prior _call_server_info_with wording is fixed (it now says the endpoint answers the record and does not read the log); code comments no longer teach that /server_info still contains "auto" or point at the 404 /get_weight_version; the new AST ratchet plus the record/overlay pair in test_server_info.py pin value sources. One new rust comment still mis-states Python's sibling source for weight_version.
Issue counts by severity
- bugs: 0
- suggestions: 1
- nits: 0
| // `RustServer.launch` REFUSES to start when it is set. It can therefore | ||
| // only be null here — echoing it keeps the field's shape. | ||
| "preferred_sampling_params": sa.preferred_sampling_params, | ||
| // Not a field of this server's `ServerArgs`: Python answers the record |
There was a problem hiding this comment.
[suggestion] The new weight_version comment says Python answers the record (default unless --weight-version or a weight update set one). That contradicts both this handler's own doc comment (Python reports the control-plane log) and the Python /model_info handler, which reads config_value("weight_version"). A weight update never writes the record; it appends to _config_updates. The rust-side fact (this blob has no weight_version, so a launch that set --weight-version still reads back null) is right; calling Python's value the record is the leftover overlay story this unit is trying to retire.
Suggestion: Say Python /model_info answers the effective value (config_value, launch record unless a control-plane write landed). Keep the rust null as a missing-field shape, not as “Python’s record.”
There was a problem hiding this comment.
Fixed. The comment now says Python answers the effective value (config_value — the launch record unless a control-plane write landed), and keeps the rust null as a missing-field shape rather than as Python's record.
d2c10c8 to
d687c92
Compare
9d350e6 to
412a28e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 412a28ea78
ℹ️ 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".
| // control-plane write landed), while a launch that set | ||
| // `--weight-version` reads back null here — a missing field, not a | ||
| // different answer to the same question. | ||
| "weight_version": serde_json::Value::Null, |
There was a problem hiding this comment.
Under SGLANG_RUST_SERVER=1, /model_info always reports weight_version: null, even though RustServer._build_server_args() serializes the complete Python ServerArgs, whose launch value defaults to "default" and may be set explicitly with --weight-version. Thus the newly added Rust parity surface gives a different answer from Python for every launch and hides an operator-supplied version; deserialize weight_version into the Rust ServerArgs and return that value here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the rust ServerArgs gained weight_version and the handler returns it.
Two details worth recording. The null predates this stack -- origin/main's handler already had serde_json::Value::Null there -- but this stack is what added the parity guard asserting the rust surface answers the same keys, so it was this stack that turned a missing field into a claim of equivalence. And the mismatch is on every launch, not only an explicit --weight-version: the field defaults to the string "default", so Python always answers a value.
_build_server_args serializes dict(vars(scheduler.server_args)), so the value was already in the blob and only the deserialization was missing. No route mounted on that server changes weights, so the launch value is also the current one -- the comment now says that instead of describing Python's answer as the record.
d687c92 to
0e1425f
Compare
412a28e to
7f77bef
Compare
0e1425f to
cabdf0d
Compare
9866c48 to
71dd6a3
Compare
cabdf0d to
d8e61f2
Compare
71dd6a3 to
2fdb271
Compare
d8e61f2 to
6596eb9
Compare
2fdb271 to
0031d22
Compare
6596eb9 to
b51825b
Compare
0031d22 to
08b496d
Compare
b51825b to
435fd65
Compare
08b496d to
3163239
Compare
435fd65 to
6fca5bd
Compare
3163239 to
833a124
Compare
/server_info overlaid the tokenizer's control-plane log (detected parsers, weight version, load format, the HiCache mirror) onto the serialized config, so the one endpoint whose job is to report what the operator asked for answered with a mix of that and what changed since. It reports the record. The facts that overlay carried keep their own endpoints: /get_weight_version and /model_info for the weight version, /hicache/storage-backend for the mirror. /model_info also reports the parsers in effect; the launcher resolves `auto` into the record before publish, so the readback already shows the resolved value. The crash dump keeps the overlay: a dump is asking what was running. is_no_spec_infer_or_topk_one is called only by the resolution pipeline, with a resolved view rather than the record. Naming its parameter after the record made it look like a supplied-instance reader, and the exposure census counted it as two pinned pairs it never was.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bea649f631
ℹ️ 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".
| // A worker that answers one endpoint and not the other still gets | ||
| // registered with whatever did answer. | ||
| let parsed = parsed.unwrap_or_default(); |
There was a problem hiding this comment.
When /model_info returns a model name but /server_info transiently exhausts its retries, this default lets register_one populate model_ids while silently losing the KV-event configuration and authoritative disaggregation role. reconcile_unresolved_workers skips every worker with nonempty model_ids, so the missing /server_info metadata is never fetched again; the worker can remain attached to the wrong PD pool or without KV-event subscriptions after the endpoint recovers. Preserve whether /server_info failed and keep such partially introspected workers eligible for reconciliation.
Useful? React with 👍 / 👎.
1 participant
/server_info overlaid the tokenizer's control-plane log (detected parsers,
weight version, load format, the HiCache mirror) onto the serialized config,
so the one endpoint whose job is to report what the operator asked for
answered with a mix of that and what changed since. It reports the record.
The facts that overlay carried keep their own endpoints: /get_weight_version
and /model_info for the weight version, /hicache/storage-backend for the
mirror. /model_info also reports the parsers in effect, which is where a
resolved-from-the-chat-template value belongs. The crash dump keeps the
overlay: a dump is asking what was running.
is_no_spec_infer_or_topk_one is called only by the resolution pipeline, with a
resolved view rather than the record. Naming its parameter after the record
made it look like a supplied-instance reader, and the exposure census counted
it as two pinned pairs it never was.
CI States
Latest PR Test (Base): 🚫 Run #32079859216
Latest PR Test (Extra): 🚫 Run #32079858982