simplify CLI arg parsing - #228
Conversation
| } | ||
| Some(arg) if !arg.starts_with("-") => { | ||
| // assume to be a port | ||
| let port = arg.parse::<u16>().ok(); |
There was a problem hiding this comment.
this is intended to match with existing behavior, but in my opinion this branch should be something like let port = arg.parse::<u16>().ok_or(format!("Invalid input for port: {arg:?}"))
![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: 69af8e9d98
ℹ️ 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".
| let port = arg.parse::<u16>().ok(); | ||
| args.next(); |
There was a problem hiding this comment.
When --prefill is followed by a malformed or out-of-range bootstrap port, such as --prefill http://prefill:8000 65536, .ok() converts the parse failure to None and the next line still consumes the argument. The router therefore accepts the typo and may silently use the default port 8998 (src/routers/http/vllm_pd_router.rs:1663) instead of refusing the invalid configuration; preserve the argument for Clap to reject or return an explicit parsing error.
Useful? React with 👍 / 👎.
69af8e9 to
86c190f
Compare
Another driveby as I was working through the codebase. Today, CLI parsing walks the list of Clap args twice. Once for prefill, then another time for non-prefill args. We can do this once using standard iterator traversal which also simpified some of the index checks needed earlier. Signed-off-by: Nikhil Thomas <nikhil@nthomas.org>
86c190f to
09ce919
Compare
1 participant
Purpose
Code legibility, general Rust-iness, and hopefully being useful to others.
Another driveby as I was working through the codebase. Today, CLI parsing walks the list of Clap args twice. Once for prefill, then another time for non-prefill args. We can do this once using standard iterator traversal which also simpified some of the index checks needed earlier.
This also fixes (I hope) a latent bug where
--prefill --port 3000would silently drop instead of making noise about a missing URL.Test Plan
cargo check, cargo fmt, clippy, unit tests (new and old)
Test Result
clean.
Essential Elements of an Effective PR Description Checklist