feat(remote): accept pre-built httpx.Client and httpx.AsyncClient - #1283
feat(remote): accept pre-built httpx.Client and httpx.AsyncClient#1283Harsh23Kashyap wants to merge 3 commits into
Conversation
Adds an optional `http_client` parameter to `QdrantClient`, `AsyncQdrantClient`, `QdrantRemote`, `AsyncQdrantRemote`, `ApiClient`, and `AsyncApiClient`. When provided, the client uses the caller's `httpx.Client` / `httpx.AsyncClient` verbatim instead of constructing its own. The user's transport, middleware, and event hooks stay under their control. The qdrant-client kwargs (limits, http2, timeout, auth) are only applied when this class builds its own httpx client. Also updates `tools/generate_async_client.sh` so the regen pipeline remaps the sync `httpx.Client` type to `httpx.AsyncClient` in the generated async signatures (the AST transformer propagates annotations verbatim and the AST itself cannot rewrite them). First half of #1068. The request-metrics half is left for a follow-up PR — that's a bigger API design call and splitting keeps this one focused. Tests: tests/test_http_client_injection.py covers the sync and async paths, including a behavioral check that rest_headers middleware stays in the chain when the caller supplies their own client.
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe sync and async Qdrant clients now accept optional pre-built Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qdrant_client/qdrant_client.py (1)
105-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent sync/async
http_clientround-tripping.
QdrantHostClientpreserveshttp_clientin_init_options, soAsyncQdrantClient(**sync_client.init_options)can pass anhttpx.Clientinto asyncAsyncApiClient, andAsyncQdrantHostClient(..., http_client=httpx.AsyncClient())can preserve an async client through sync construction.ApiClient.send()ultimately callsself._client.send(request), while async REST methods awaitclient.send(), so keep the option or rebuildhttp_clientfor the target client type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qdrant_client/qdrant_client.py` around lines 105 - 115, Update QdrantHostClient initialization and init_options handling so sync/async conversions never pass an httpx.Client to AsyncApiClient or an httpx.AsyncClient to the sync client. Exclude incompatible http_client instances from round-tripped options or rebuild them for the target client type, while preserving compatible clients. Ensure AsyncQdrantClient(**sync_client.init_options) and the reverse remain valid through ApiClient.send.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qdrant_client/http/api_client.py`:
- Around line 68-81: The ApiClient and AsyncApiClient wrappers currently close
caller-owned HTTP clients during cleanup. Track whether each client was created
internally from the `http_client is None` condition, and update
`ApiClient.close()` and `AsyncApiClient.aclose()` to close only internally owned
clients while leaving injected clients untouched.
In `@qdrant_client/qdrant_remote.py`:
- Around line 234-239: The remote upload worker paths must not receive
caller-owned httpx clients through _rest_args. In qdrant_client/qdrant_remote.py
lines 234-239 and qdrant_client/async_qdrant_remote.py lines 190-196, prevent
injected http_client values from being passed to ParallelWorkerPool arguments,
or reject parallel uploads when one is supplied; preserve direct client usage
for non-worker requests and apply the same ownership rule to both sync and async
uploaders.
---
Outside diff comments:
In `@qdrant_client/qdrant_client.py`:
- Around line 105-115: Update QdrantHostClient initialization and init_options
handling so sync/async conversions never pass an httpx.Client to AsyncApiClient
or an httpx.AsyncClient to the sync client. Exclude incompatible http_client
instances from round-tripped options or rebuild them for the target client type,
while preserving compatible clients. Ensure
AsyncQdrantClient(**sync_client.init_options) and the reverse remain valid
through ApiClient.send.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff97c666-9b5b-448d-a69e-13c8a934363f
📒 Files selected for processing (7)
qdrant_client/async_qdrant_client.pyqdrant_client/async_qdrant_remote.pyqdrant_client/http/api_client.pyqdrant_client/qdrant_client.pyqdrant_client/qdrant_remote.pytests/test_http_client_injection.pytools/generate_async_client.sh
Three follow-ups from the bot review of #1283: - `ApiClient.close()` / `AsyncApiClient.aclose()` now track whether the underlying httpx client was built internally or supplied by the caller, and only close the internally-built one. The caller owns their client's lifecycle and may keep using it after the qdrant client is closed. - The user-supplied http_client is no longer placed in `self._rest_args`. That dict is also forwarded to per-process upload workers (`_upload_collection` -> `RestBatchUploader`); a shared client across forked workers is unsafe (httpx connection pools don't survive fork) and would serialize workers on the shared connection pool. The client is now passed as a direct kwarg to the main `SyncApis` / `AsyncApis` constructor only. - `http_client` is excluded from `_init_options` capture in `QdrantClient` and `AsyncQdrantClient`. The round-trip path (`AsyncQdrantClient(**sync._init_options)`) would otherwise leak a sync httpx.Client into the async client (or vice versa) and fail at runtime. Callers who want an http_client on both sides pass it explicitly to each constructor; non-http_client args (timeout, headers, etc.) still round-trip. New tests cover: `QdrantClient.close()` not closing the injected client, the built-client path still closing, the round-trip exclusion, and the async counterparts.
|
Re: the round-trip concern about httpx.Client leaking into AsyncQdrantClient and vice versa — fixed in be6c321. http_client is now excluded from _init_options capture in both QdrantClient and AsyncQdrantClient. The round-trip path (AsyncQdrantClient(**sync._init_options)) no longer carries the http_client over. Callers who want an http_client on both sides pass it explicitly to each constructor; non-http_client args (timeout, headers, etc.) still round-trip. Two new tests cover this: test_init_options_round_trip_excludes_http_client (sync) and the async counterpart. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_http_client_injection.py`:
- Around line 87-104: The lifecycle tests currently bypass ownership assertions.
In tests/test_http_client_injection.py:87-104, assert the injected client
remains open after QdrantClient.close() instead of swallowing probe exceptions;
at 105-121, assert the constructor-set _owns_client value without overwriting
it; and at 138-144, await client.close() and verify the default-created async
client is closed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 975d81b7-1e13-450b-9181-1293ad902496
📒 Files selected for processing (6)
qdrant_client/async_qdrant_client.pyqdrant_client/async_qdrant_remote.pyqdrant_client/http/api_client.pyqdrant_client/qdrant_client.pyqdrant_client/qdrant_remote.pytests/test_http_client_injection.py
🚧 Files skipped from review as they are similar to previous changes (4)
- qdrant_client/qdrant_remote.py
- qdrant_client/qdrant_client.py
- qdrant_client/http/api_client.py
- qdrant_client/async_qdrant_client.py
- test_close_does_not_close_injected_client: assert `not injected.is_closed` directly, instead of swallowing probe exceptions. Also assert the constructor set `_owns_client = False`. - test_close_does_close_built_client: stop monkey-patching the `_owns_client` flag. Assert the constructor set it to True, then verify the default-built client is closed by `api_client.close()`. - test_default_path_still_builds_its_own_async_client: actually call `client.close()` and verify the default-built `httpx.AsyncClient` is closed, instead of just calling `aclose()` on the inner client directly. - test_aclose_does_not_close_injected_client: mirror the sync ownership check (`_owns_client is False` + `is_closed`). All 12 tests still pass.
1 participant
Summary
Adds an optional
http_clientparameter toQdrantClient,AsyncQdrantClient,QdrantRemote,AsyncQdrantRemote,ApiClient, andAsyncApiClient. When provided, the client uses the caller'shttpx.Client/httpx.AsyncClientverbatim instead of constructing its own. Backward compatible — the default path is unchanged.Motivation
QdrantRemotebuilds its ownhttpx.Client/httpx.AsyncClientfrom the constructor kwargs (limits,http2,timeout,auth, ...). Callers who need to attach middleware, event hooks, custom transports, or observability instrumentation (Prometheus exporters, OpenTelemetry hooks) currently have no way to do that.This unblocks the most common use case from #1068: a custom
httpx.Clientwith transport-level instrumentation.What's in this PR
The injection half of #1068. Plumbing the user-supplied client through every layer that builds one internally.
ApiClient.__init__/AsyncApiClient.__init__: newhttp_client: Client | None = None/http_client: AsyncClient | None = Noneparameter. When set, used as-is. WhenNone, the existingClient(**kwargs)/AsyncClient(**kwargs)path runs.QdrantRemote.__init__/AsyncQdrantRemote.__init__: newhttp_clientparameter, forwarded toApiClient/AsyncApiClientvia_rest_args["http_client"]after the existing kwargs are processed.QdrantClient.__init__/AsyncQdrantClient.__init__: newhttp_clientparameter, forwarded toQdrantRemote/AsyncQdrantRemote. Theinit_optionsround-trip preserves it (sync -> async or async -> sync).tools/generate_async_client.sh: asedstep that remapshttpx.Clienttohttpx.AsyncClientin the regeneratedasync_qdrant_remote.pyandasync_qdrant_client.pysignatures. The AST transformer that produces the async files propagates parameter annotations from the sync source verbatim, so the newhttp_client: httpx.Clientannotation in the sync source would leak into the async signature without this fix. Future regens stay consistent with the manually-edited files.What's NOT in this PR
The request-metrics half of #1068. That's a real API design call (which metrics, sync vs per-request, what surface) and deserves its own focused discussion. Open to a follow-up PR once this injection lands.
Design notes
When the caller supplies an
http_client, the qdrant-client constructor kwargs (limits,http2,timeout,auth) are not applied to it. The caller owns their client; this matches the standardhttpxconvention where you build the client once and let it own its transport. Comments in the source make this explicit.The
rest_headersmiddleware is still attached on top of the user-supplied client — observability of the qdrant-client does not regress as a side effect of opting in. Covered by a behavioral test.ApiClient/AsyncApiClientare exported types fromqdrant_client.http, so the new parameter is part of the public surface for anyone constructing them directly.Test plan
tests/test_http_client_injection.pycovers:httpx.Clientis the actual instance used byQdrantClient._client.openapi_client.client._client(identity check, notisinstance).QdrantRemotelevel directly, bypassing theQdrantClientfacade.httpx.Client(regression check).rest_headersmiddleware (behavioral test: set a context header, drive the middleware, assert the header is stamped onto the request)._init_options["http_client"]is preserved, soQdrantClient(http_client=...)->AsyncQdrantClient(**sync._init_options)round-trip works.httpx.AsyncClient.Existing
tests/test_tracing.pystill passes (14/14).mypy qdrant_client/qdrant_remote.py qdrant_client/qdrant_client.py qdrant_client/async_qdrant_remote.py qdrant_client/async_qdrant_client.pyis clean (the four non-testsfiles mypy actually checks;qdrant_client/http/is excluded bymypy.ini).ruff checkis clean on all changed files. The 3 pre-existingF541warnings inqdrant_remote.py/async_qdrant_remote.pyare not introduced by this PR.ruff formatonqdrant_client/http/api_client.pywould touch unrelated pre-existing overload signatures; left alone to keep the diff focused.Out of scope
async-client-consistency-check.shfailure on master (the regen produces ~50 lines of unrelated formatter drift in the async files; not introduced here). I'll send a separate PR for that if useful.Backward compatibility
Purely additive. All new parameters default to
None, the default path constructs a freshhttpx.Client/httpx.AsyncClientexactly as before. No public signatures changed, no public behavior changed for callers that don't passhttp_client.Fixes #1068