Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe changes add context-manager support across sync and async client classes: QdrantBase now implements AbstractContextManager with enter/exit, and AsyncQdrantBase implements AbstractAsyncContextManager with aenter/aexit; many concrete client/local/remote classes gain corresponding enter or aenter methods. Tests were added to validate context-manager close behavior. The async client generation tooling was extended to allow method renaming (e.g., "enter" → "aenter") via a new rename_methods pipeline, and transformers were updated with guards to avoid redundant class/constant replacements and to honor renamed method mappings. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/test_async_qdrant_client.py (6)
🪛 Ruff (0.14.2)tests/test_async_qdrant_client.py600-600: Abstract (TRY301) 600-600: Avoid specifying long messages outside the exception class (TRY003) ⏰ 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)
🔇 Additional comments (1)
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 (8)
qdrant_client/local/qdrant_local.py (1)
1262-1263: Redundant enter overrideQdrantLocal inherits QdrantBase, which already provides enter/exit. This override is harmless but unnecessary. Consider dropping it to avoid duplication.
qdrant_client/local/async_qdrant_local.py (1)
1166-1167: Consider relying on AsyncQdrantBase for aenterIf AsyncQdrantBase implements AbstractAsyncContextManager, this method is redundant. Keeping just base-provided aenter/aexit reduces duplication.
Can you confirm AsyncQdrantBase provides aenter/aexit that call close()?
qdrant_client/client_base.py (1)
513-523: Context manager base looks good; minor typing nitenter/exit are correct and do not suppress exceptions. Optionally, annotate exit as -> Optional[bool] to mirror AbstractContextManager, though returning None is fine.
tools/async_client_generator/transformers/constant_transformer.py (1)
13-14: Safer predicate for string replacementCurrent guard works, but also checking old_value presence avoids unnecessary replace calls and makes intent clearer:
- if isinstance(node.value, str) and new_value not in node.value: - node.value = node.value.replace(old_value, new_value) + if ( + isinstance(node.value, str) + and old_value in node.value + and new_value not in node.value + ): + node.value = node.value.replace(old_value, new_value)tests/test_async_qdrant_client.py (1)
584-592: Avoid poking private _client in testsAccessing client._client.closed couples tests to internals. Expose a public closed property on AsyncQdrantClient (proxying to the underlying client) and assert on that instead. Also consider a test that raises inside the context to ensure aexit calls close() on exceptions.
Example addition outside this file:
# qdrant_client/async_qdrant_client.py @property def closed(self) -> bool: return getattr(self._client, "closed", False)tools/async_client_generator/transformers/class_def_transformer.py (1)
12-13: Guard is good; consider handling dotted base namesThe new guard prevents double-replacement. For completeness, also handle dotted bases:
for base in node.bases: for old_value, new_value in self.class_replace_map.items(): - if hasattr(base, "id"): - base.id = base.id.replace(old_value, new_value) + if isinstance(base, ast.Name): + base.id = base.id.replace(old_value, new_value) + elif isinstance(base, ast.Attribute): + base.attr = base.attr.replace(old_value, new_value)Also applies to: 16-20
qdrant_client/qdrant_remote.py (1)
3452-3453: Delegate enter to the base implementation.QdrantBase.enter now guards against re-entering a closed client; bypassing it skips that safeguard. Call super().enter before returning self.
def __enter__(self) -> "QdrantRemote": - return self + super().__enter__() + return selfqdrant_client/async_qdrant_remote.py (1)
3159-3160: Let AsyncQdrantBase.aenter run first.The async base class handles the closed-client guard; await super().aenter() before returning self to keep that protection.
async def __aenter__(self) -> "AsyncQdrantRemote": - return self + await super().__aenter__() + return self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
qdrant_client/async_client_base.py(2 hunks)qdrant_client/async_qdrant_client.py(1 hunks)qdrant_client/async_qdrant_remote.py(1 hunks)qdrant_client/client_base.py(2 hunks)qdrant_client/local/async_qdrant_local.py(1 hunks)qdrant_client/local/qdrant_local.py(1 hunks)qdrant_client/qdrant_client.py(1 hunks)qdrant_client/qdrant_remote.py(1 hunks)tests/test_async_qdrant_client.py(1 hunks)tests/test_qdrant_client.py(1 hunks)tools/async_client_generator/base_client_generator.py(3 hunks)tools/async_client_generator/client_generator.py(3 hunks)tools/async_client_generator/fastembed_generator.py(2 hunks)tools/async_client_generator/local_generator.py(3 hunks)tools/async_client_generator/remote_generator.py(2 hunks)tools/async_client_generator/transformers/class_def_transformer.py(1 hunks)tools/async_client_generator/transformers/client/function_def_transformer.py(1 hunks)tools/async_client_generator/transformers/constant_transformer.py(1 hunks)tools/async_client_generator/transformers/function_def_transformer.py(2 hunks)tools/async_client_generator/transformers/remote/function_def_transformer.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
qdrant_client/async_client_base.py (3)
qdrant_client/async_qdrant_client.py (2)
http(208-216)close(160-167)qdrant_client/async_qdrant_remote.py (2)
http(370-376)close(215-236)qdrant_client/local/async_qdrant_local.py (1)
close(73-89)
tests/test_qdrant_client.py (3)
qdrant_client/qdrant_client.py (3)
QdrantClient(27-3176)delete_collection(2301-2319)create_collection(2321-2409)qdrant_client/local/qdrant_local.py (3)
delete_collection(971-986)create_collection(988-1039)closed(70-71)qdrant_client/qdrant_remote.py (3)
delete_collection(2749-2762)create_collection(2764-2883)closed(256-257)
tools/async_client_generator/transformers/remote/function_def_transformer.py (2)
tools/async_client_generator/transformers/client/function_def_transformer.py (1)
_keep_sync(20-24)tools/async_client_generator/transformers/function_def_transformer.py (1)
_keep_sync(17-18)
tools/async_client_generator/base_client_generator.py (10)
tools/async_client_generator/transformers/import_from_transformer.py (1)
ImportFromTransformer(5-21)tools/async_client_generator/transformers/import_transformer.py (1)
ImportTransformer(5-13)tools/async_client_generator/transformers/call_transformer.py (1)
CallTransformer(5-23)tools/async_client_generator/transformers/function_def_transformer.py (1)
FunctionDefTransformer(6-47)tools/async_client_generator/transformers/class_def_transformer.py (1)
ClassDefTransformer(5-20)tools/async_client_generator/transformers/constant_transformer.py (1)
ConstantTransformer(5-15)tools/async_client_generator/client_generator.py (1)
async_methods(56-59)tools/async_client_generator/fastembed_generator.py (1)
async_methods(40-43)tools/async_client_generator/local_generator.py (1)
async_methods(55-58)tools/async_client_generator/remote_generator.py (1)
async_methods(88-108)
tools/async_client_generator/transformers/client/function_def_transformer.py (2)
tools/async_client_generator/transformers/function_def_transformer.py (1)
_keep_sync(17-18)tools/async_client_generator/transformers/remote/function_def_transformer.py (1)
_keep_sync(22-26)
tests/test_async_qdrant_client.py (3)
qdrant_client/async_qdrant_client.py (2)
AsyncQdrantClient(26-3033)get_collections(2143-2150)qdrant_client/async_qdrant_remote.py (2)
get_collections(2357-2374)closed(212-213)qdrant_client/local/async_qdrant_local.py (2)
get_collections(860-868)closed(70-71)
qdrant_client/client_base.py (8)
qdrant_client/async_qdrant_client.py (2)
http(208-216)close(160-167)qdrant_client/async_qdrant_remote.py (2)
http(370-376)close(215-236)qdrant_client/qdrant_client.py (2)
http(226-235)close(175-182)qdrant_client/qdrant_remote.py (2)
http(421-427)close(259-280)qdrant_client/local/async_qdrant_local.py (1)
close(73-89)qdrant_client/local/qdrant_local.py (1)
close(73-92)qdrant_client/async_client_base.py (1)
close(474-475)qdrant_client/http/api_client.py (2)
close(56-57)close(139-140)
tools/async_client_generator/fastembed_generator.py (2)
tools/async_client_generator/transformers/fastembed/call_transformer.py (1)
FastembedCallTransformer(5-14)tools/async_client_generator/transformers/fastembed/function_def_transformer.py (1)
FastembedFunctionDefTransformer(4-9)
⏰ 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 (7)
qdrant_client/qdrant_client.py (1)
3175-3176: No action required; context manager protocol is completeQdrantClient inherits from QdrantFastembedMixin, which inherits from QdrantBase. QdrantBase defines
__exit__(client_base.py:516–522) and inherits from AbstractContextManager. The__enter__method in QdrantClient paired with the inherited__exit__from QdrantBase provides a working context manager protocol. No additional__exit__definition is needed.tools/async_client_generator/transformers/remote/function_def_transformer.py (1)
16-18: Review comment is based on incorrect assumption; code is safeThe parent
FunctionDefTransformerclass explicitly initializesself.rename_methodsto an empty dict whenNoneis passed (line 14 offunction_def_transformer.py), guaranteeing thatself.rename_methods.get()in_keep_sync(lines 23-26) will never fail. All required imports (Optional,Any,show_warning) are already present inasync_qdrant_remote.py, and thehttp.aclose()pattern is already in use elsewhere in the same file (line 229). No action needed.Likely an incorrect or invalid review comment.
qdrant_client/async_qdrant_client.py (1)
3032-3033: LGTM! Correct async context manager implementation.The
__aenter__method correctly returnsself, enabling the standardasync with AsyncQdrantClient(...) as client:pattern. This pairs with the__aexit__method (implemented in the base class per the PR summary) to provide proper resource cleanup when exiting the context.Note: This file is autogenerated per the header warning (lines 1-10). Any issues should be addressed in the generator scripts rather than this file directly.
tests/test_qdrant_client.py (1)
1990-2013: LGTM! Context manager test appropriately validates core behavior.The test correctly verifies that both HTTP and local clients properly close when used as context managers, checking the
_client.closedflag after exiting thewithblock.The intentional simplification (noted in the comment at lines 2011-2012) is reasonable: the underlying
close()behavior is already extensively tested intest_client_close(), and the context manager protocol simply delegates to that shared implementation.tools/async_client_generator/remote_generator.py (1)
50-56: rename_methods propagation looks good.Wiring rename_methods through RemoteFunctionDefTransformer ensures the async generation retains the desired method renames.
tools/async_client_generator/local_generator.py (1)
19-45: Local generator wiring LGTM.Accepting and forwarding rename_methods matches the other generators and keeps the async codegen consistent.
tools/async_client_generator/transformers/client/function_def_transformer.py (1)
8-24: Nice rename-aware keep_sync logic.Accounting for rename_methods when deciding whether to keep a sync variant avoids dropping renamed async methods.
1 participant
This PR adds support for using the respective qdrant clients as a contextmanager (i.e. as part of
withstatements). The required__enter__/__aenter__operation is implemented as no-op, while__exit__/__aexit__call theclose()operation. While this is not strictly necessary for the sync clients (as these can be wrapped withcontextlib.closingproviding the same functionality), this does not work for async clients (as the correspondingcontextlib.aclosingrequires aaclosemethod to be present). This requires either a custom wrapper or the proposed change.While the changes to the clients itself are quite simple, there is a little bit more going on in the generator code. We try to achieve both renaming the function defs as well as keeping return type covariance. Suggestions there are especially welcome.
All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
--> There are some tests concerning locks that fail locally. As these fail on
devas well, I suppose this is unrelated.pre-commitwithpip3 install pre-commitand set up hooks withpre-commit install?