Repository navigation
Conversation
Release reserved slots in finally blocks and keep asynchronous capacity waiting cancellable. Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
yashrajp22
left a comment
There was a problem hiding this comment.
I found no actionable introduced defect in the scoped review of head 6df3025 against base b6f0304. All 65 focused HEAD pool checks passed in both dependency modes, covering construction/invocation failures and cancellation. The pool code was tested from contrib source in both modes; it is not shipped in the wheel.
The runtime evidence remains qualified: the harness continued after a supplemental preflight failure, so the recorded launch-sequencing deviation still applies. Provider calls were mocked. Combined callbacks-on-retry, live cancellation/transport cleanup, load performance and fairness, other Python/platform combinations, and the full repository suite were not verified. Nine of the 12 corpus samples per mode remained partial, with signature exclusions retained; this is not a global accuracy result. This summary applies to the pinned comparison above; integration with newer main has not been reviewed.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@YMuskrat thank you for the precise analysis of how client-construction failures and cancellation could leak pool slots, and for the cancellation-focused tests.
Value and readiness
The source change is now superseded. #796 (7ee43713, merged to main today) rewrote PooledChatModel._invoke_with_retry and _ainvoke_with_retry with the same design:
_build_llmis called inside thetry.- A single
finally: self._pool.release(key, success=not rate_limited)releases the slot. - The async path polls
try_acquire()with a cancellableasyncio.sleep(0.05)instead ofasyncio.to_thread(self._pool.acquire). - It also adds a shared request deadline.
This PR now conflicts with main in contrib/batch_scan/api_pool.py. Main already has test_pooled_request_releases_slot_when_client_build_fails (sync and async) and deadline tests, but no async cancellation tests. Your in-flight cancellation, waiter cancellation, release-then-proceed and one-release-per-retry cases would still add regression value.
Material findings
- Blocker:
contrib/batch_scan/api_pool.py:464. The PR conflicts with #796 and duplicates its fix. Please rebase onto main, drop theapi_pool.pychanges, and keeptests/test_batch_scan_pool_cleanup.pyas a test-only change against main's implementation. When rebasing:- check the tests against main's signatures:
_build_llm(key, *, timeout=...)(yourMockstubs accept it),release(key, success=...), and the default 30 s deadline; - drop or merge
test_model_construction_failure_releases_slot, which duplicates main's existing test; - update the comment about the "old thread-based acquire", since that path no longer exists.
- check the tests against main's signatures:
PIC tradeoffs
None identified. Whether a test-only follow-up is wanted, or the PR should simply be closed, is a maintainer preference.
Verification and gaps
- Compared this diff with main's
api_pool.pyafter #796. The release, cancellation and polling semantics match, and main additionally bounds waiting with_remaining(deadline). AsyncCancelledError(aBaseException) still runs main'sfinally, so in-flight cancellation releases the slot on main. git merge-treeagainst mainb07f181dreports a content conflict inapi_pool.py. Searched main's batch tests forCancelledErrorandtask.canceland found none, which is the gap these tests would fill.- yashrajp22's COMMENT review found no defect at this head against an older base. It does not consider #796.
- CI is green on the old base. I did not run the tests (static review).
Decision: Changes Requested (reviewed head 6df30258b2306432a0d5a45e13767875bd691f0c)
Keep cancellation, capacity recovery and one-release-per-retry coverage against main; drop duplicate client-construction cases. Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
#796 already fixed the pool cleanup, so this is now a tests-only follow-up. It covers cancelling a running request or a waiting request, reusing a freed slot, and releasing each slot once across sync/async rate-limit retries.
Dropped the duplicate construction-failure tests and updated the threading comment.
api_pool.pyis now identical tomain.Checks: 41 focused tests pass, plus lint and formatting. Four existing process tests also fail on unchanged
mainhere. The pool tests use mocked provider calls.