Replace ThreadPool with ThreadPoolExecutor for async_req#633
Open
yonatan-genai wants to merge 3 commits intopinecone-io:mainfrom
Open
Replace ThreadPool with ThreadPoolExecutor for async_req#633yonatan-genai wants to merge 3 commits intopinecone-io:mainfrom
yonatan-genai wants to merge 3 commits intopinecone-io:mainfrom
Conversation
…sync_req ThreadPool depends on POSIX named semaphores (sem_open), which fails on platforms without /dev/shm such as AWS Lambda. The async_req code path only needs thread-based parallelism, not multiprocessing. This reuses the existing threadpool_executor property (already used by the async_threadpool_executor path) for async_req as well. A .get() alias is added to the returned Future for backward compatibility with code that calls .get() instead of .result(). Removes the multiprocessing dependency from the client entirely. <claude>
The .get() alias is monkey-patched onto Future for external backward compat. Internal code should call .result() directly.
Author
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1c55cc9. Configure here.
Future.result() would leak through __getattr__ and return the raw OpenAPI response without adapting it. Since the PR exposes a dual .get()/.result() API on futures, users may try .result() on the transformer too — it should return the transformed UpsertResponse.
Author
|
Fixed in ce21f28 — added an explicit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
async_req=Truecode path usesmultiprocessing.pool.ThreadPool, whichdepends on POSIX named semaphores (
sem_open). This fails on platforms without/dev/shm, notably AWS Lambda, Android, and Docker containers with restrictedshared memory.
ThreadPoolis only used for thread-based parallelism here, not multiprocessing.The
ThreadPoolExecutor(fromconcurrent.futures) is already used internallyby the
async_threadpool_executorpath. This change reuses it forasync_reqas well, eliminating the
multiprocessingdependency entirely.A
.get()alias is added to the returnedFuturefor backward compatibilitywith existing code that calls
.get(timeout)(theApplyResultAPI) insteadof
.result(timeout)(theFutureAPI).Changes
pinecone/openapi_support/api_client.py: Replaceself.pool.apply_async()with
self.threadpool_executor.submit(), remove ThreadPool property andmultiprocessing imports
pinecone/adapters/response_adapters.py: UpdateUpsertResponseTransformerfrom
ApplyResulttoFutureRelated
Note
Medium Risk
Touches core async request plumbing by swapping
ThreadPool.apply_async()forThreadPoolExecutor.submit()and emulating the old.get()API; regressions could surface in timeouts, cancellation, or code that relied onApplyResultsemantics.Overview
Replaces the
async_req=Trueexecution path to useconcurrent.futures.ThreadPoolExecutorinstead ofmultiprocessing.pool.ThreadPool, removing themultiprocessing/POSIX semaphore dependency and improving compatibility with restricted runtimes (e.g., Lambda/containers without/dev/shm).Updates async return types accordingly:
ApiClient.call_api()now returns aFuturewith a backward-compatible.get()alias, andUpsertResponseTransformernow wraps aFutureand provides bothget()andresult()to return the transformed SDKUpsertResponse.Reviewed by Cursor Bugbot for commit ce21f28. Bugbot is set up for automated code reviews on this repo. Configure here.