Repository navigation
test(grpc): cover TokenSpeed and SGLang server-info label conversion - #1654
Conversation
ModelInfo::to_labels and ServerInfo::to_labels feed the worker labels surfaced by GET /workers for gRPC-connected workers, but had no unit coverage. Add tests for the TokenSpeed and SGLang arms: curated server_args key picking, integer formatting, version mapping, and the flat_labels skip/keep rules (empty strings, zero numbers, false booleans, JSON-encoded arrays). Also document in the admin API reference that /get_server_info and /get_model_info are HTTP-worker passthroughs: gRPC workers surface the same metadata as labels in GET /workers (discovered at registration via the GetServerInfo/GetModelInfo RPCs), with live scheduler state on /get_loads. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates API documentation and adds unit test coverage for gRPC worker label extraction. The ChangesgRPC Worker Label Extraction and Testing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
👋 The PR description doesn't fully follow PULL_REQUEST_TEMPLATE.md:
Please update the PR description so reviewers have the context they need. |
There was a problem hiding this comment.
Code Review
This pull request updates the API documentation to clarify that /get_model_info and /get_server_info are proxied to HTTP workers, adding notes on how gRPC-connected workers are handled. It also introduces unit tests in model_gateway/src/routers/grpc/client.rs to verify the serialization of server and model info into labels for TokenSpeed and Sglang backends. The review feedback suggests improving test code maintainability by introducing a bool_value helper function to simplify the construction of boolean values in tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn number_value(n: f64) -> prost_types::Value { | ||
| prost_types::Value { | ||
| kind: Some(prost_types::value::Kind::NumberValue(n)), | ||
| } | ||
| } |
There was a problem hiding this comment.
To complete the set of test helper functions for prost_types::Value types and avoid verbose inline construction of boolean values later in the tests, consider adding a bool_value helper function alongside string_value and number_value.
fn number_value(n: f64) -> prost_types::Value {
prost_types::Value {
kind: Some(prost_types::value::Kind::NumberValue(n)),
}
}
fn bool_value(b: bool) -> prost_types::Value {
prost_types::Value {
kind: Some(prost_types::value::Kind::BoolValue(b)),
}
}References
- Extract duplicated logic into a shared helper function to improve maintainability and reduce redundancy.
| ( | ||
| "is_embedding".to_string(), | ||
| prost_types::Value { | ||
| kind: Some(prost_types::value::Kind::BoolValue(false)), | ||
| }, | ||
| ), |
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f62125359
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn server_info_to_labels_tokenspeed_picks_curated_keys_and_version() { | ||
| let info = ServerInfo::TokenSpeed(Box::new(tokenspeed_proto::GetServerInfoResponse { | ||
| server_args: Some(prost_types::Struct { | ||
| fields: BTreeMap::from([ |
There was a problem hiding this comment.
Use HashMap for prost Struct fields
When these unit tests are compiled, prost_types::Struct::fields expects a HashMap<String, prost_types::Value>, but the new fixtures initialize it with BTreeMap::from(...). That type mismatch prevents the test target from compiling anywhere routers::grpc::client::tests is built, and the same pattern repeats for the other Struct fixtures in this module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Solid test coverage for the gRPC worker-label conversion layer. Tests correctly verify curated-key picking, version mapping, skip/keep rules (empty strings, zero numbers, booleans, arrays), and exclusion of non-curated keys and runtime state. Doc updates are accurate.
Description
Follow-up to #1574 (closed). For gRPC-connected workers, backend server/model
info is not served by a router endpoint: the worker registration workflow calls
the backend's
GetModelInfo/GetServerInfoRPCs (discover_metadata.rs) andsurfaces the results as worker labels in
GET /workers. The TokenSpeed arms ofthat pipeline landed with #1351, but the label-conversion layer
(
ModelInfo::to_labels/ServerInfo::to_labels) had no unit coverage, and theadmin API docs did not say where this data lives in gRPC mode.
Changes
routers/grpc/client.rs:ServerInfo: curatedTOKENSPEED_GRPC_KEYSpicked from theserver_argsStruct, integral numbers formatted without a decimal point,tokenspeed_version→version, unlisted keys /scheduler_info/transient runtime state excluded.
ServerInfo: same contract viaSGLANG_GRPC_KEYS, includingis_embedding=falsekept as a label (embedding detection relies on it) andnon-curated keys (e.g.
api_key) excluded.ModelInfo:flat_labelsskip/keep rules — empty strings andzero numbers skipped,
falsebooleans kept, arrays JSON-encoded.docs/reference/api/admin.md: note on/get_server_infoand/get_model_infothat they are HTTP-worker passthroughs; gRPC workers exposethe same metadata as labels in
GET /workers, with live scheduler state on/get_loads.Test Plan
Summary by CodeRabbit