Repository navigation
feat(workflow): surface max_running_requests on SGLang HTTP /server_info - #1529
Conversation
|
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 (1)
📝 WalkthroughWalkthrough
ChangesServer Metadata Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Code Review
This pull request adds the max_running_requests field to the ServerInfo struct in discover_metadata.rs to surface the per-instance concurrency cap for SGLang. It also includes unit tests to verify the deserialization and label extraction of this new field, ensuring it handles both present and missing values correctly. I have no feedback to provide.
There was a problem hiding this comment.
Clean, well-scoped change. The new max_running_requests field on ServerInfo is correctly typed as Option<usize>, matches the SGLang JSON field name (no rename needed), and is auto-surfaced by the existing generic flat_labels pipeline. Tests cover both the present and absent cases. No issues found.
ab192ae to
02421cb
Compare
There was a problem hiding this comment.
Clean, minimal change. The new max_running_requests: Option<usize> field correctly closes the HTTP-only gap (gRPC path already had it at SGLANG_GRPC_KEYS). flat_labels picks it up automatically via serde — no manual wiring needed. Tests cover both present and absent cases. No issues found.
SMG's curated ServerInfo struct deserializes a subset of SGLang's /server_info response (~800 fields). max_running_requests — the CLI flag --max-running-requests, which is the natural per-instance batch capacity cap — was being dropped silently. The SGLang gRPC label pipeline already extracts this field (see SGLANG_GRPC_KEYS in routers/grpc/client.rs). Adding it here closes the HTTP-only path so capacity-aware consumers see the same label regardless of transport. Two unit tests verify both the present-and-absent cases; flat_labels picks the field up automatically via its serde-based serialization. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
02421cb to
bf42012
Compare
Summary
ServerInfostruct indiscover_metadata.rsdeserializes a subset of SGLang's/server_inforesponse (which has ~800 fields).max_running_requests— the per-instance batch-capacity cap from--max-running-requests— was being silently dropped.pub max_running_requests: Option<usize>to the struct.flat_labelsis serde-based and picks it up automatically — no other changes needed.SGLANG_GRPC_KEYSinrouters/grpc/client.rs) already extracts this field; this closes the HTTP-only path so capacity-aware consumers see the same label regardless of transport.Test plan
cargo test --package smg --lib workflow::steps::local::discover_metadata::tests::test_sglang_server_info— 2 new unit tests pass (present-and-absent cases).#[ignore]integration tests against a live SGLang server still pass.workflowtest suite.Notes
vllm_engine.protoGetServerInfoResponsehas no batch-capacity field, so this fix is SGLang-HTTP-specific. TRT-LLM usesmax_batch_size(different name); that's a separate, larger change.Summary by CodeRabbit
New Features
Tests