[None][fix] validate trtllm-serve --port with ValueError not assert#16260
[None][fix] validate trtllm-serve --port with ValueError not assert#16260lonexreb wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthrough
ChangesServe port validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/commands/serve.py`:
- Around line 290-293: Move the port validation in the serve command before the
socket.getaddrinfo(host, port, ...) call so invalid negative ports raise the
intended ValueError. Preserve the existing rule that port must be greater than
zero unless disagg_cluster_config is provided, while allowing port == 0 only
with that configuration.
In `@tests/unittest/llmapi/apps/test_serve_port_validation.py`:
- Around line 20-30: Expand
test_launch_server_rejects_nonpositive_port_without_disagg to cover both port=0
and port=-1 using parametrization, and mock or spy on the server socket bind
operation to assert bind() is never called when validation fails. Preserve the
ValueError assertion and existing launch_server inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c31e2072-7296-421c-867c-0eeb27d649df
📒 Files selected for processing (2)
tensorrt_llm/commands/serve.pytests/unittest/llmapi/apps/test_serve_port_validation.py
|
Addressed in 26826eb: parametrized over port 0 and -1 (mocking socket so getaddrinfo doesn't fail first on -1) and assert bind() is never called when validation fails. |
JunyiXu-nv
left a comment
JunyiXu-nv
left a comment
There was a problem hiding this comment.
Changes are small and good! But we want to hold this PR for a while before this one #15815 get merged. So that we can add the test into the cpu only test stage to make it covered by CI.
launch_server guarded the --port CLI argument with an assert, which raises AssertionError and, under python -O, is stripped entirely (silently binding an ephemeral port). Raise a ValueError instead. Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
Parametrize over port 0 and -1 (mock socket so getaddrinfo does not fail first for -1) and assert bind() is never called when validation fails. Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
JunyiXu-nv
force-pushed
the
fix/serve-port-validation
branch
from
26826eb to
18de818
Compare
JunyiXu-nv
left a comment
JunyiXu-nv
left a comment
There was a problem hiding this comment.
Hi @lonexreb , could you please add the test into tests/integration/test_lists/test-db/l0_cpu_x86.yml? And then we can run CI and merge this PR. Thanks!
2 participants
Description
launch_server(tensorrt_llm/commands/serve.py) guarded thetrtllm-serve --portCLI argument with:Using
asserton user input means anAssertionError(not a clear error), and underpython -Othe check is stripped entirely — so--port 0without a disagg config silently binds an ephemeral port instead of erroring. Behavior should not depend on the-Oflag.Change
Replace the
assertwith an explicitraise ValueError.Test
New CPU-only unit test in
tests/unittest/llmapi/apps/test_serve_port_validation.py:launch_serverwithport=0and no disagg config raisesValueError(the check runs before any bind/model load).Summary by CodeRabbit
ValueErrormessage instead of relying on an assertion.