Repository navigation
Fix async event loop and task lifecycle - #1507
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
👋 Review Summary
This PR introduces a coherent set of async lifecycle improvements across perf benchmarks, model async clients, sandbox service, external bridge streaming, and Toolathlon relays. The intent is clear: align event-loop ownership with resource lifetimes, supervise producer/consumer pipelines, and make shutdown/cancellation predictable. The changes are internally consistent and backed by targeted tests.
🛡️ Key Risks & Issues
- Perf benchmark pipeline and strategies: The new
run_benchmark_pipelineflow (evalscope/perf/core/metrics_consumer.py:138) correctly coordinates producer, queue drain, and consumer tasks, with explicit failure checks and cleanup viacancel_and_wait. Closed-loop and multi-turn strategies now structure task creation and cancellation so in-flight work is cleaned up on errors. I did not identify race conditions or leaks here; the main behavioral change is that early consumer exit or producer failure surfaces as a runtime error instead of being silently swallowed. - Async model client lifecycle: The loop-bound client pool (evalscope/models/utils/async_client.py:17) and
ModelAPI.aclose()hooks make ownership explicit and ensure async clients are closed on their owner loops. The runtime error when attempting cleanup after the owner loop is stopped is intentional and encourages callers to callModel.aclose()before shutting down loops. The logic for owned vs external loops is sound, and new tests exercise the core invariants. - External bridge streaming and Toolathlon relay: Streaming handlers in the bridge now await generation task cancellation via
cancel_and_wait(evalscope/agent/external/bridge/server.py:433, 464, 688, 771), reducing the risk of pending tasks or partially torn-down sessions after disconnects. Toolathlon’srun_ws_proxyhas been restructured around a sharedhttpx.AsyncClient, supervised request processing, heartbeat monitoring, and coordinated cancellation (evalscope/benchmarks/toolathlon/client.py:357). The error and timeout paths look robust, and the tests cover several important scenarios. - Event-loop ownership and shutdown semantics:
AsyncioLoopThread,AsyncioLoopRunner, andModelProxyServernow enforce that close callbacks and bridge shutdown happen on the owning loop. This is a stricter, safer contract that prevents cleanup on the wrong loop. The tests verify that callbacks run on the expected loops, async generators are finalized, pending tasks are cancelled, and default executors are drained.
I did not find logic bugs, security risks, or severe performance issues that would block this PR.
🧪 Verification Advice
- Run the documented perf and CLI smoke tests on environments that mimic your production setup (including uvloop or custom loop runners) to confirm there are no regressions in long-running sweeps or multi-benchmark runs.
- Exercise scenarios where benchmarks are run sequentially and concurrently, watching for warnings about pending tasks, closed event loops, or unclosed HTTP sessions; the new lifecycle code should keep logs clean.
- Verify that external bridges and Toolathlon relays behave correctly under mid-stream failures (remote disconnect, heartbeat timeouts, websocket send errors) and that no background tasks are left running after the run completes.
- In integrations that supply their own event loop (e.g., web frameworks, notebooks), explicitly call
Model.aclose()/ModelAPI.aclose()before shutting down the loop and confirm that cleanup completes without errors.
💡 Thoughts & Suggestions
- Consider adding a small number of integration-style tests that run a full evaluation involving perf, async model clients, sandbox, external bridges, and Toolathlon together, then assert that there are no remaining tasks or open loops/sessions after completion. This would reinforce the async lifecycle guarantees this PR is aiming for.
- It may be useful to document, at a high level, the new expectation that callers should invoke
Model.aclose()before stopping event loops that host async clients, especially for users embedding evalscope into their own async runtimes. - Overall, this is a substantial and well-structured improvement to the async lifecycle story in evalscope, and the added tests significantly increase confidence in the changes.
🤖 Generated by Qoder • View workflow run
6b55a4f to
1e5e588
Compare
Summary
Fix the async lifecycle issues found while reproducing #1504:
ModelAPI.aclose()/Model.aclose()lifecycle for caller-managed loopsRoot cause
The original failure came from mismatched resource and event-loop lifetimes. A cached Volcengine sandbox manager and its async transports were created on a worker-owned loop, but that loop was closed after the sample. Later samples reused resources still bound to the closed loop.
The same ownership class appeared in adjacent paths:
id(loop)and canceled streaming generation without awaiting itChanges
Owned event loops
stop()shutdown_asyncgens()andshutdown_default_executor()Perf
SystemExitFileExistsErrorfor an existing result databaseAioHttpClient.__aenter__()returnselfand keep close idempotentExternal bridge
Toolathlon
httpx.AsyncClientfor the WebSocket relay lifetimefinallyand await every canceled task before exitPublic behavior
run_perf_benchmark()now raises the original Python exception instead of exiting the processdata_process_completed_eventexport is removedasyncio.run()calls in synchronous CLI entry points remain unchangedValidation
122 passed, 10 skipped66 passed, 8 skipped8 passed1 passedconda run -n eval make lintmean_acc=1.0,mean_acc_pass@1=1.0Fixes #1504