[CI] Fix race condition in test_kv_cache_events test #18169
Conversation
We observed an occasional failure on this test case: tests/v1/engine/test_engine_core_client.py::test_kv_cache_events[True-tcp] I believe this change should resolve it. The KV event publisher was initializing its zmq socket in a new thread, which gives a window of opportunity for the subscriber created in the test to try to connect too soon, putting things in a bad state and missing the event expected. The `time.sleep(0.1)` in the test is a pretty good hint that this is a problem. I assume whoever put that there observed this race and found that this yield seemed to be enough to fix it. Unfortunately this sort of thing is usually still bound to fail on occassion. The fix here is to move zmq socket initialization back into the main flow of control prior to creating the background thread. That way we know it's created before the test creates the subscriber. The change also removes the sleep, as it should no longer be necessary if this race condition was the reason it was added originally. Signed-off-by: Russell Bryant <rbryant@redhat.com>
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
|
@russellb - can you unskip the test in the CI? |
vllm-project#18169 fixed vllm-project#18167 Signed-off-by: David Xia <david@davidxia.com>
|
@robertgshaw2-redhat, #18183 will reinstate the test |
4 participants
We observed an occasional failure on this test case:
tests/v1/engine/test_engine_core_client.py::test_kv_cache_events[True-tcp]
I believe this change should resolve it. The KV event publisher was
initializing its zmq socket in a new thread, which gives a window of
opportunity for the subscriber created in the test to try to connect too
soon, putting things in a bad state and missing the event expected.
The
time.sleep(0.1)in the test is a pretty good hint that this is aproblem. I assume whoever put that there observed this race and found
that this yield seemed to be enough to fix it. Unfortunately this sort
of thing is usually still bound to fail on occassion.
The fix here is to move zmq socket initialization back into the main
flow of control prior to creating the background thread. That way we
know it's created before the test creates the subscriber. The change
also removes the sleep, as it should no longer be necessary if this race
condition was the reason it was added originally.
Signed-off-by: Russell Bryant rbryant@redhat.com