Repository navigation
feat(worker): add max_running_requests accessor on Worker trait - #1526
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)
📝 WalkthroughWalkthroughAdds a default Worker::max_running_requests() that reads "max_running_requests" from WorkerMetadata.spec.labels, parses it as u16, and returns None for missing, non-numeric, or zero values. Unit tests cover valid, missing, non-numeric, and "0" cases. ChangesWorker max_running_requests capacity
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 |
|
Hi @slin1237, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2915272894
ℹ️ 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".
| .spec | ||
| .labels | ||
| .get("max_running_requests") | ||
| .and_then(|s| s.parse::<u16>().ok()) |
There was a problem hiding this comment.
Parse max_running_requests with full int32 range
This parser uses u16, but the upstream scheduler schemas expose max_running_requests as int32 (for example in crates/protocols/src/worker.rs), so any valid value above 65,535 will be treated as parse failure and silently converted to None. In deployments that set very high concurrency limits, this drops real capacity data and forces downstream logic to fall back as if the worker never reported a limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces the max_running_requests method to the Worker trait, enabling the gateway to derive worker capacity from metadata labels, and includes comprehensive unit tests for this new functionality. The reviewer suggested enhancing the robustness of the label parsing by adding a .trim() call to handle potential whitespace and reminded the author to ensure the metadata discovery pipeline is updated to populate these fields.
| fn max_running_requests(&self) -> Option<u16> { | ||
| self.metadata() | ||
| .spec | ||
| .labels | ||
| .get("max_running_requests") | ||
| .and_then(|s| s.parse::<u16>().ok()) | ||
| .filter(|n| *n > 0) | ||
| } |
There was a problem hiding this comment.
Instead of widening the return type to usize, maintain the more restrictive internal protocol type (e.g., u16) when mapping from external sources like labels. This aligns with the repository's preference for safe conversions over changing internal protocols, especially when the backend might not support wider types. Additionally, use .trim() before parsing to make the accessor more resilient to accidental whitespace in labels.
Note: To fulfill the intent of being "populated by metadata discovery", ensure that the discovery pipeline (e.g., the ServerInfo and ModelsResponseEntry structs in discover_metadata.rs) is updated to include these fields so they can be captured into labels.
| fn max_running_requests(&self) -> Option<u16> { | |
| self.metadata() | |
| .spec | |
| .labels | |
| .get("max_running_requests") | |
| .and_then(|s| s.parse::<u16>().ok()) | |
| .filter(|n| *n > 0) | |
| } | |
| fn max_running_requests(&self) -> Option<u16> { | |
| self.metadata() | |
| .spec | |
| .labels | |
| .get("max_running_requests") | |
| .and_then(|s| s.trim().parse::<u16>().ok()) | |
| .filter(|n| *n > 0) | |
| } |
References
- When mapping a numeric type from an external API to a more restrictive internal protocol type, prefer a safe conversion that drops out-of-range values over changing the internal protocol.
There was a problem hiding this comment.
Clean addition. The accessor is well-documented, well-tested, and the zero-filtering behavior is a sensible design choice. The u16 range question raised by another reviewer is worth considering but not blocking — 65,535 concurrent requests per worker is well beyond realistic capacity.
2915272 to
eb51ae1
Compare
eb51ae1 to
c15b1a1
Compare
Reads the 'max_running_requests' label populated by metadata discovery. Used by WorkerCapacity (PR 2 in this series) to derive fleet capacity. Returns None when the label is missing, unparseable, or zero. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
c15b1a1 to
8247eee
Compare
Summary
max_running_requests(&self) -> Option<u16>default method on theWorkertrait that reads themax_running_requestslabel populated by metadata discovery (vLLM--max-num-seqs, SGLang--max-running-requests).Nonewhen the label is missing, unparseable, or zero (zero is meaningless for capacity accounting).WorkerCapacitycomponent will consume to derive fleet-wide in-flight capacity fromWorkerRegistry.Test plan
cargo test --package smg --lib worker::worker::tests::test_max_running_requests— 4 new tests pass (label present / missing / unparseable / zero).cargo clippy --all-targets --all-features -- -D warnings— clean.worker::*tests.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests