ITADN

verl tool adapter: synchronous env execution blocks the agent-loop event loop

#2571Closedaniruddh-alt 创建于 22 天前
A
aniruddh-altcommented
## Problem `OumiVerlTool.execute` is `async`, but the work it does is synchronous: ```python router = get_or_build_router(agent_data, _parent_router(self._env_config_path)) result = router.route_batch([(self._tool_id, args)])[0] ``` `route_batch` → `ExecutableEnvironment.step` → the executor runs inline on verl's asyncio worker. verl schedules many trajectories (and parallel tool calls within a turn) on one event loop, so a single slow tool call stalls every rollout sharing that worker. The first call of a rollout is worse: it also builds the environment (for a `schema_sql` database env, that materializes a snapshot and runs DDL + seed). Not a problem for the current NL2SQL consumer — SQLite queries against a staged Spider DB are milliseconds. It matters for any environment whose `step` does slow I/O: an HTTP/MCP-backed tool, a browser env, a large seed script. ## Why `asyncio.to_thread` is not the fix `DatabaseSession` opens its connection without `check_same_thread=False`: ```python self.connection = sqlite3.connect(self._path, isolation_level=None) ``` so the connection is pinned to the thread that created it. A default thread-pool offload can land subsequent calls on a different worker thread and raise `ProgrammingError`. The rollback-on-close transaction semantics also assume one owning thread. ## Sketch of a real fix Give each rollout router a dedicated single thread that both **builds** and **drives** its environments, and await it from `execute`: - one `ThreadPoolExecutor(max_workers=1)` per rollout router, created alongside the router in `env_provider` - `for_sample(...)` and every `route_batch(...)` for that rollout run on that thread — preserving per-router serialization and thread affinity - shut the executor down in `_teardown` next to `router.close()` Per-router (not global) keeps one rollout's slow call from blocking another's, which is the actual goal. ## Validation this needs A cluster GRPO run to confirm throughput improves (or at least doesn't regress) and that DB isolation still holds — the current synchronous path is validated end to end (Spider val EX 0.367 → 0.684). ## Context Raised by a Codex review of #2560; deliberately deferred out of that PR to keep it reviewable.
关闭于 20 天前 0 条评论