fix(tests): use bare --router-request-timeout-secs in glm4.7 mooncake test - #77
Conversation
… test The hybrid router-arg convention (settled in #18/#40) keeps the vllm_ prefix only for the endpoint-locating flags --vllm-router-ip and --vllm-router-port; every other vllm-router knob is passed bare with a --router- prefix (dest router_*). The glm4.7 PD/mooncake integration test still passed the pre-hybrid name --vllm-router-request-timeout-secs, which is no longer a registered flag, so the 1200s timeout never reached router_args.request_timeout_secs. This test is GPU-only and outside the unit suite, so the rename sweep in #40 missed it. Rename the flag to --router-request-timeout-secs to match arguments.py (dest router_request_timeout_secs) and rollout.py's consumer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the test script tests/test_glm4.7_30B_A3B_pd_mooncake.py by renaming the command-line argument --vllm-router-request-timeout-secs to --router-request-timeout-secs. There are no review comments, and I have no feedback to provide.
| "--vllm-max-num-seqs 16 " | ||
| "--vllm-max-cudagraph-capture-size 8 " | ||
| "--vllm-speculative-config '{\"method\":\"mtp\",\"num_speculative_tokens\":3}' " | ||
| "--vllm-router-request-timeout-secs 1200 " |
There was a problem hiding this comment.
I remember that we plan to just replace sglang by vllm. And slime is using --sglang-router-request-timeout-secs. See https://github.com/THUDM/slime/blob/7a7aba4eea36513e3c076594d3aa9aa6263ce558/slime/backends/sglang_utils/arguments.py#L25
There was a problem hiding this comment.
I remember that we plan to just replace sglang by vllm. And slime is using
--sglang-router-request-timeout-secs. See https://github.com/THUDM/slime/blob/7a7aba4eea36513e3c076594d3aa9aa6263ce558/slime/backends/sglang_utils/arguments.py#L25
So --router-request-timeout-secs is a genuine vllm-router knob. We register the router package's entire CLI surface (~30 args — policy, cache_threshold, retries, health-check, timeout, …) via RouterArgs.add_cli_args(parser, use_router_prefix=True), which namespaces them all under --router-* and reads them back from args.router_. So this flag's name and dest come straight from vllm-router. Renaming it --vllm-router- would desync it from the other ~30 router knobs and from the dest that RouterArgs.from_cli_args reads.
--vllm-router-ip / --vllm-router-port are different: they aren't vllm-router CLI args at all (we pass exclude_host_port=True) — they're vime's own "where do I reach the router" flags, so they live in vime's --vllm-* namespace.
So the rule is: --router-* = pass-through to the router package, --vllm-* = vime-owned. What do you think, I'm also consider renaming vllm-router-port and ip to router-port and ip as well.
There was a problem hiding this comment.
I agree with remove some redundant vllm- prefix.
… test (#77) The hybrid router-arg convention (settled in #18/#40) keeps the vllm_ prefix only for the endpoint-locating flags --vllm-router-ip and --vllm-router-port; every other vllm-router knob is passed bare with a --router- prefix (dest router_*). The glm4.7 PD/mooncake integration test still passed the pre-hybrid name --vllm-router-request-timeout-secs, which is no longer a registered flag, so the 1200s timeout never reached router_args.request_timeout_secs. This test is GPU-only and outside the unit suite, so the rename sweep in #40 missed it. Rename the flag to --router-request-timeout-secs to match arguments.py (dest router_request_timeout_secs) and rollout.py's consumer. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
What
One-line fix: tests/test_glm4.7_30B_A3B_pd_mooncake.py passed the pre-hybrid flag
--vllm-router-request-timeout-secs 1200. Renamed to bare--router-request-timeout-secs 1200.Why
The hybrid router-arg convention settled in #18 / #40:
vllm_prefix:--vllm-router-ip,--vllm-router-port(vime owns where the router lives; RouterArgs excludes host/port).--router-*(destrouter_*), e.g.--router-request-timeout-secs,--router-policy.This GPU-only integration test still used the old
--vllm-router-request-timeout-secs, which is no longer a registered flag. The 1200s timeout therefore never reachedrouter_args.request_timeout_secs(slime/ray/rollout.py:978 reads bareargs.router_request_timeout_secs). Because the test is outside the unit suite, #40's rename sweep missed it.Scope
--vllm-router-*non-ip/port flags remain intests/.tests/unit/backends/vllm_utils/) stays green: 66 passed.🤖 Generated with Claude Code