Skip to content

Conversation

rapsealk
Copy link
Member

Follow-up of {#5223, #5253} and resolves #5820 (BA-2325)

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue

@rapsealk rapsealk added this to the 25.13 milestone Sep 12, 2025
@github-actions github-actions bot added size:M 30~100 LoC comp:client Related to Client component size:S 10~30 LoC and removed size:M 30~100 LoC labels Sep 12, 2025
@github-actions github-actions bot added size:M 30~100 LoC size:S 10~30 LoC and removed size:S 10~30 LoC size:M 30~100 LoC labels Sep 12, 2025
if aiohttp_session is not None:
self.aiohttp_session = aiohttp_session
self._aiohttp_session_injected = True
self._owns_session = False
Copy link
Collaborator

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.

Copy link
Member Author

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.

@github-actions github-actions bot added size:L 100~500 LoC comp:common Related to Common component and removed size:S 10~30 LoC labels Sep 12, 2025
ctx.run(loop.run_until_complete, self.agen_wrapper(coro))
else:
try:
# FIXME: Once python/mypy#12756 is resolved, remove the type-ignore tag.

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@rapsealk rapsealk marked this pull request as ready for review September 12, 2025 06:14
@Copilot Copilot AI review requested due to automatic review settings September 12, 2025 06:14
@rapsealk rapsealk modified the milestones: 25.13, 25.14 Sep 12, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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.

@rapsealk rapsealk force-pushed the feature/BA-2325-sync-session-connection-pool branch from f6a7542 to d5507ec Compare September 16, 2025 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:client Related to Client component comp:common Related to Common component size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add connection pooling support to synchronous API session
2 participants