-
Notifications
You must be signed in to change notification settings - Fork 164
feat(BA-2325): Apply client pool to synchronous API session #5841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/ai/backend/client/session.py
Outdated
if aiohttp_session is not None: | ||
self.aiohttp_session = aiohttp_session | ||
self._aiohttp_session_injected = True | ||
self._owns_session = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the name "own_session" is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it as _has_aiohttp_session_ownership
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements client pool functionality for synchronous API sessions, following up on previous work in issues #5223 and #5253. The changes introduce a new synchronous client pool implementation and refactor the existing session management to support both async and sync client pooling.
- Modernizes type annotations from legacy typing module to built-in types
- Creates shared sync worker thread utilities for managing async operations in sync contexts
- Implements SyncClientPool for connection pooling in synchronous API sessions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/ai/backend/common/types.py | Updates type annotations to use built-in types and adds Sentinel enum documentation |
src/ai/backend/common/sync.py | New module providing SyncWorkerThread for async-to-sync operation bridging |
src/ai/backend/common/clients/http_client/client_pool.py | Refactors client pool architecture with base class and adds SyncClientPool implementation |
src/ai/backend/client/types.py | Removes duplicate Sentinel implementation, now using common version |
src/ai/backend/client/session.py | Refactors session classes to use shared sync utilities and support injected aiohttp sessions |
changes/5841.feature.md | Adds changelog entry for the feature |
Comments suppressed due to low confidence (1)
src/ai/backend/common/sync.py:1
- The
execute
method is designed for coroutines that complete and return a value, but_cleanup_loop
is an infinite loop coroutine. This will block the worker thread indefinitely and prevent it from processing other work items. Consider using a separate task or thread for the cleanup loop, or restructure the design to avoid blocking the worker thread.
import asyncio
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
f6a7542
to
d5507ec
Compare
Follow-up of {#5223, #5253} and resolves #5820 (BA-2325)
Checklist: (if applicable)