fix: give progress startup its own timeout budget - #8
Conversation
`start()` waited `_JOIN_TIMEOUT_SECONDS` (10s) for the progress thread to
become ready. That constant describes how long `close()` should wait for a
thread to *join*; reusing it as a cold-start budget conflates two unrelated
waits.
Startup is not bounded by anything KVCR controls. Before setting `_ready`,
the thread creates the NIXL agent, initializes backends, registers memory,
and captures agent metadata. UCX enumerates every network device inside
`ucp_init`, so the cost scales with the host, and ranks sharing a node pay it
concurrently.
Measured in the quick-start image, `nixl_agent()` alone:
host network (~220 veth interfaces), 1 agent 11.8 - 14.6s
bridge network (2 interfaces), 1 agent 1.7s
host network, 2 agents concurrently 10.3 - 11.0s
host network, 4 agents concurrently 22.0 - 24.2s
host network, 8 agents concurrently 35.5 - 41.0s
Memory registration is not a factor; UCX defers it, and registering 32GB
measured under 10ms.
The documented quick-start requires `--network host`, which is exactly the
configuration that exposes every interface. Both engine ranks failed with:
RuntimeError: KVCR progress thread did not start
The thread had not failed. It finished ~23s in and logged a healthy agent
well after `start()` gave up, leaving resources behind through the
non-quiescent retention path.
Introduce `_STARTUP_TIMEOUT_SECONDS` (30s) for the readiness wait and leave
`_JOIN_TIMEOUT_SECONDS` to mean what its name says. Track the stage the
thread is in so a genuine hang names it instead of claiming the thread never
started.
30s is a deliberately conservative 3x increase that covers the failure above
and the measured 1-4 rank cases. It does not cover 8 ranks initializing
concurrently on one node, which measured 35-41s; that case wants either a
larger budget or a configurable one, and is left for a follow-up so this
change stays a minimal fix with no public API surface.
The existing test asserted the old message and forced the timeout by
patching `_JOIN_TIMEOUT_SECONDS`; it now patches the startup budget, and
keeps patching the join budget so `close()` does not wait on a thread the
test parks on purpose.
Signed-off-by: Kang Zhang <kangz@nvidia.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b25f84b to
fcffb94
Compare
|
Thank you for this pr! cc @hxieustc and @mkhazraee could you please take a look? |
|
@kangclzjc the timeout budgets are hardcoded in the python scripts, however, for different systems & setups, the timeout budgets are likely to differ. Did you consider to make them configurable via the kvconnector configs, while keeping the default timeout budgets when such configs are not specified in CLI? For instance, { |
Yes, I'll add this field via kvconnector configs, currently I just increase the value because this PR are not merged in vllm |
|
Hi Kang, thanks for the PR. There were several changes, so I incorporated the logging idea and further clarified time out names in PR #26 . Let me know if it covers what you wanted to achieve or something is left. |
Fixes #5
start()waited_JOIN_TIMEOUT_SECONDS(10s) for the progress thread to become ready. That constant describes how longclose()should wait for a thread to join; reusing it as a cold-start budget conflates two unrelated waits.Changes
_STARTUP_TIMEOUT_SECONDS(30s) for the readiness wait, leaving_JOIN_TIMEOUT_SECONDSto mean what its name says.No
KVCRConfigfield, deliberately — that is public API and would have to be threaded through the vLLM adapter in vllm-project/vllm#53624. This keeps the change to zero API surface.Measurements
Startup is not bounded by anything KVCR controls. UCX enumerates every network device inside
ucp_init, so NIXL agent creation scales with the host, and ranks sharing a node pay that cost concurrently.nixl_agent()alone, measured in the quick-start image:nixl_agent()vethinterfaces), 1 agentMemory registration is not a factor — UCX defers it, and registering 32GB measured under 10ms.
Both vLLM data-parallel ranks failed with
RuntimeError: KVCR progress thread did not start, ~23s before the agent actually came up.docs/quick-start.mdrequires--network host, which is exactly the configuration that exposes every interface.On the choice of 30s
30s is a deliberately conservative 3x increase over the current 10s. It covers the observed failure (23s) and the measured 1–4 rank cases.
It does not cover 8 ranks initializing concurrently on one node, which measured 35–41s — an ordinary shape on an 8-GPU box. That case wants either a larger budget or a configurable one. It is left for a follow-up so this change stays a minimal fix with no public API surface; reviewers who would rather raise the constant now should say so and I will adjust.
A complementary option, not included here: periodic progress logging during startup, so a slow-but-healthy start is visible rather than silent. The stage tracking added here is the hook for it.
Test change
test_startup_timeout_retains_nonquiescent_resourcesasserted the old message and forced the timeout by patching_JOIN_TIMEOUT_SECONDS. It now patches the startup budget, and asserts the reported stage so the new phase tracking is covered.It also keeps patching
_JOIN_TIMEOUT_SECONDS. That is load-bearing: the test parks the agent on purpose, so without itclose()spends its full join budget waiting for a thread that will not move, adding 20s to the suite.Verification
ruff check src testspasses.pytest tests/unitmatchesmainexactly — 152 passed, 14.5s, same 15 pre-existing environmental failures (SO_PEERPIDFDneeds Linux 6.5+, this host runs 5.15; CI runners are unaffected).🤖 Generated with Claude Code