Repository navigation
fix(ai): fail vLLM prompt queries on request errors instead of hanging - #7643
TanishGudise wants to merge 2 commits into
Conversation
LocalVLLMExecutor._generate only decremented running_task_count after a successful generate, and the futures holding the exception were dropped, so one failing request left VLLMSink::finalize polling forever. Decrement in a finally, keep the first exception (including cancellation), and re-raise it from poll() and all_tasks_finished(). _periodic_log was created after loop.run_forever() returned, so it never ran. Start it on the first threaded submit(), and stop it once a request fails or all requests are done. Co-Authored-By: Claude Opus 5.5 <[email protected]>
 [Medium impact] The changes affect the vLLM prompt workflow in local runs and Ray actors. The PR appears safe to merge, with a non-blocking cleanup issue in the test fixture. Findings
SummaryThe PR makes failed vLLM requests fail the query instead of leaving it waiting forever.
The only finding is non-blocking test cleanup: the fixture should cancel pending tasks and close its loop. Acknowledged scope: TanishGudise explicitly defers the existing Ray routing problem with multiple input partitions as a separate issue. TanishGudise also states that Ray keeps the existing per-request logging fallback.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def executor(fake_vllm): | ||
| executor = LocalVLLMExecutor("fake-model", {}, {}) | ||
| yield executor | ||
| executor.loop.call_soon_threadsafe(executor.loop.stop) |
There was a problem hiding this comment.
The executor fixture stops its loop without cancelling and awaiting pending tasks or closing the loop. Tests that call submit() leave _periodic_log sleeping when teardown runs. This can produce cleanup warnings and leave open loop resources for garbage collection.
Cancel and await pending tasks before stopping the loop, then join the thread and close the loop.
There was a problem hiding this comment.
Fixed in 62140b6: the fixture now cancels and awaits pending tasks, stops the loop, joins the thread and closes it (dev-mode warnings 4 → 0; vLLM tests 5/5).
…ture The fixture only stopped the executor's loop, so `_periodic_log` was left pending and the loop was never closed (ResourceWarning: unclosed event loop). Cancel and await the pending tasks, stop the loop, join its thread and close it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Changes Made
If one request fails,
prompt(..., provider="vllm-prefix-caching")never finishes. Locally the error is never shown; on Ray it only shows up in the actor log as "Task exception was never retrieved".Repro (CPU only, fake engine, in the new
tests/ai/vllm/test_vllm_executor.py): three prompts, andAsyncLLMEngine.generate()raises for one of them. On main,df.collect()never returns:VLLMSink::finalizepollsall_tasks_finished()every 500 ms and it staysFalse.Cause:
LocalVLLMExecutor._generatedecrementsrunning_task_countonly after a successful generate, and nothing reads the exception, because therun_coroutine_threadsafefutures (and, in Ray actors, thecreate_tasktasks) are dropped.Fix:
_generatedecrements the count in afinallyand keeps the first exception, including cancellation;poll()andall_tasks_finished()re-raise it. The query now fails with the engine's own exception (on Ray with single-partition input, wrapped inRayTaskError). The re-raise matters: with only thefinally, the query finishes with 2 of 3 rows and no error._periodic_log()was created afterloop.run_forever(), which only returns once the loop stops, so it never ran. It now starts on the firstsubmit(), on the same loop as the requests. It updateslast_log_timeso the per-request fallback rarely logs twice, and it stops once a request has failed, or once submitting is finished and no requests are running. Ray actors usesubmit_asyncand keep the per-request fallback, as before.Tests: 6 new tests with a fake
vllmmodule, no GPU needed. On main, 5 of them fail (the end-to-end one hits its 30 s timeout); with this change all 6 pass. I also ran the full native suite locally on this exact code: 60,433 passed, and every failure but one also fails on main in the same environment (mostly tests that need network access, model downloads, S3/minio or generated TPC-H data). The exception,tests/test_subscribers.py::test_capture_states(a SIGINT timing test), passes 3 of 3 times when rerun alone, on this branch and on main.Not changed here: on Ray, a query whose input has more than one partition can return fewer rows than it got or hang, with or without a failing request (e.g. 4 of 40 rows after
.repartition(4)with a fake engine and no failures), and then the error from this change may not surface either. That looks like a separate problem in howPrefixRoutertracks executors; I can file it separately.AI disclosure: I used Claude while writing this fix and its tests; I reviewed every line, ran the full suite locally, and can answer questions about any of it.
Related Issues
Closes #7642