Fix/lazy load local mode#1134
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request defers portalocker imports from module-global scope to runtime-local scope in both Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 0
🧹 Nitpick comments (2)
qdrant_client/local/async_qdrant_local.py (1)
795-803: Ruff B905 onzip()in upload_collection: consider explicitstrictor suppressRuff flags the
zip()at lines 818–820 (B905: “zip() without an explicit strict=”). Here the behavior intentionally allows truncation using an infinite UUID generator:for point_id, vector, payload in zip( ids or uuid_generator(), iter(vectors), payload or itertools.cycle([{}]) ): ...If your minimum supported Python version includes
zip(..., strict=...), you can keep semantics and satisfy Ruff by making this explicit:- for point_id, vector, payload in zip( + for point_id, vector, payload in zip( ids or uuid_generator(), iter(vectors), payload or itertools.cycle([{}]), + strict=False, ):If you still support Python versions without
strict, the alternative is to keep the current behavior and silence B905 for this line with a targeted# noqa: B905comment.Please pick whichever matches your Python version policy.
Also applies to: 807-821
qdrant_client/async_qdrant_fastembed.py (1)
738-742: Consider addingstrict=Truetozip()for clarity.While the slicing at lines 738-739 ensures
dense_responsesandsparse_responseshave equal lengths, addingstrict=Trueto thezip()call would make this invariant explicit and help catch bugs if the slicing logic changes in the future.Since this is an autogenerated file, this improvement should be applied to the source file or generation script.
Apply this pattern in the source:
for dense_response, sparse_response in zip(dense_responses, sparse_responses, strict=True)Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
qdrant_client/async_qdrant_client.py(1 hunks)qdrant_client/async_qdrant_fastembed.py(6 hunks)qdrant_client/async_qdrant_remote.py(13 hunks)qdrant_client/local/async_qdrant_local.py(11 hunks)qdrant_client/local/qdrant_local.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
qdrant_client/async_qdrant_fastembed.py (8)
qdrant_client/qdrant_fastembed.py (2)
_get_model_params(214-231)embedding_model_name(93-96)qdrant_client/local/qdrant_local.py (1)
query_batch_points(426-447)qdrant_client/async_qdrant_client.py (1)
query_batch_points(209-249)qdrant_client/async_qdrant_remote.py (1)
query_batch_points(471-513)qdrant_client/local/async_qdrant_local.py (1)
query_batch_points(386-404)qdrant_client/async_client_base.py (1)
query_batch_points(42-45)qdrant_client/client_base.py (1)
query_batch_points(32-38)qdrant_client/qdrant_remote.py (1)
query_batch_points(549-594)
qdrant_client/async_qdrant_remote.py (1)
qdrant_client/qdrant_remote.py (2)
_try_argument_to_grpc_selector(1305-1336)_try_argument_to_rest_points_and_filter(1389-1416)
qdrant_client/local/async_qdrant_local.py (1)
qdrant_client/local/qdrant_local.py (1)
_resolve_query_input(238-339)
🪛 GitHub Actions: Integration tests
qdrant_client/async_qdrant_client.py
[error] 107-110: Generated async_qdrant_client.py is not consistent with file in this repository, see diff above.
🪛 Ruff (0.14.8)
qdrant_client/async_qdrant_fastembed.py
742-742: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
qdrant_client/local/async_qdrant_local.py
818-820: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
🔇 Additional comments (6)
qdrant_client/local/qdrant_local.py (1)
83-89: Lazy portalocker import in local mode is correct and achieves the PR goalMoving
import portalockerinsideclose()and_load()while guarding onself._flock_filepreserves the existing locking behavior for persistent local storage, but prevents portalocker (and its temp-dir checks) from running at module import time. That fixes the read-only-container crash for users who never touch local mode, without changing semantics for those who do.This looks good as-is.
Also applies to: 132-147
qdrant_client/local/async_qdrant_local.py (2)
84-89: Async local portalocker lazy import and query/prefetch resolution look correct
- The lazy
import portalockerinclose()and_load()matches the syncQdrantLocalpattern and keeps portalocker (and its temp-dir checks) off the import path unless persistent local mode is actually used._resolve_query_inputand its use in_resolve_prefetch_input,query_points, andquery_points_groupsnow mirror the synchronous implementation’s behavior for ID→vector resolution and “mentioned ids” filtering.All of this aligns with the intended semantics; no issues spotted.
Also applies to: 128-138, 224-315, 343-349, 369-373, 435-439
637-641: Comprehension and unpacking style cleanups are safeThe updated comprehensions over
self.aliases.items()andself.collections.items()just drop redundant tuple parentheses. Behavior and returned structures are unchanged.No action needed here.
Also applies to: 648-652, 658-662, 709-712
qdrant_client/async_qdrant_remote.py (1)
87-88: Selector helper usage and unpacking/style changes are consistent and correct
- The minor unpacking/comprehension tweaks around URL parsing and headers (
self._host, self._port,_rest_headers,_parse_url) are purely stylistic and don’t alter behavior.- Routing all points/shard-key handling in
delete_vectors,delete,set_payload,overwrite_payload,delete_payload, andclear_payloadthrough_try_argument_to_grpc_selector,_try_argument_to_rest_selector, and_try_argument_to_rest_points_and_filtermatches the synchronousqdrant_remotelogic and makes async behavior consistent for all supportedPointsSelectorforms (lists, REST/grpc selectors, filters, etc.).- The inferred
opt_shard_key_selectormerge (shard_key_selector = shard_key_selector or opt_shard_key_selector) is the same pattern used in the sync client and looks correct.No problems found here.
Also applies to: 119-119, 228-233, 1053-1076, 1269-1295, 1318-1357, 1370-1407, 1420-1455, 1467-1475
qdrant_client/async_qdrant_client.py (1)
105-111: This review comment appears to be based on incorrect assumptions about the codebase. After thorough investigation:
- No
tools/generate_async_client.shscript exists in the repository- The
async_qdrant_client.pyfile contains no markers, comments, or evidence that it is autogenerated- No CI workflow or check for "generated async client is consistent" was found
- The dict comprehension code at lines 105–111 is identical between the sync (
qdrant_client.py) and async (async_qdrant_client.py) clientsThe changes to lines 105–111 are standard Python code without any generation mechanism attached. If concerns about client consistency exist, please clarify the specific generation process and CI check being referenced.
Likely an incorrect or invalid review comment.
qdrant_client/async_qdrant_fastembed.py (1)
283-283: LGTM! Style improvements align with Python conventions.These changes remove redundant parentheses from tuple unpacking, which is the idiomatic Python style. The logic remains unchanged.
Note: This file is autogenerated (see header comments), so these style improvements originate from the source file or generation script.
Also applies to: 397-397, 439-439, 460-460, 654-654, 742-742
* Fix: Lazy load QdrantLocal to support read-only envs * Fix: Lazy load QdrantLocal to support read-only envs * Fix: Regenerate async client using Python 3.10 to match CI * chore: add comments to portalocker import --------- Co-authored-by: Kirstin <[email protected]> Co-authored-by: George Panchuk <[email protected]>
2 participants
Description
Fixes issue where the client crashes immediately upon import in read-only Docker containers (such as AWS EKS or Lambda) due to
portalockerinitialization checks.The Issue
Previously,
qdrant_client.local.qdrant_localimportedportalockerat the top level.portalockerimmediately checks for a writable temporary directory upon import. In read-only filesystems (where/tmpmight not be writable or mounted), this raised aFileNotFoundError, crashing the application even if the user only intended to connect to a remote Qdrant server and never used the local storage.The Fix
Moved
import portalockerfrom the global scope inqdrant_client/local/qdrant_local.pyandqdrant_client/local/async_qdrant_local.pyinto the specific methods that require file locking (_loadandclose).All Submissions:
devbranch. Did you create your branch fromdev?Changes to Core Features: