Add if_not_exists parameter to create_collection#1149
Add if_not_exists parameter to create_collection#1149veeceey wants to merge 4 commits intoqdrant:masterfrom
Conversation
- Add if_not_exists boolean parameter to create_collection() method in both QdrantClient and AsyncQdrantClient - When if_not_exists=True, only create collection if it doesn't already exist - Prevents need for manual collection_exists() check before creation - Add comprehensive tests for sync and async clients - Maintain backward compatibility with default if_not_exists=False Fixes qdrant#1022
✅ 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 Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds an optional parameter Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
All checks passing, ready for maintainer review |
|
Thanks for the review, CodeRabbit! Good suggestion on adding a negative test case for |
Verify that creating an existing collection without if_not_exists=True raises a ValueError, confirming the default behavior is preserved. Covers both sync and async clients. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Added negative test cases as discussed — both sync and async tests now verify that creating an existing collection without |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_create_collection_if_not_exists.py (2)
29-29: Cleanup calls are not guaranteed to run on assertion failure — preferpytest.fixtureortry/finallyEach test calls
delete_collection(orawait delete_collection) as the last statement. If any precedingassertfails, that call is skipped. While this has no practical inter-test impact here because every test spins up a fresh:memory:client, it establishes a fragile pattern that will break if tests are ever refactored to share a client or run against a real server.A
pytest.fixturewithyieldis the idiomatic fix:♻️ Example refactor using `pytest.fixture`
import pytest from qdrant_client import QdrantClient, models from qdrant_client.async_qdrant_client import AsyncQdrantClient `@pytest.fixture` def sync_client(): client = QdrantClient(":memory:") yield client client.close() `@pytest.fixture` async def async_client(): client = AsyncQdrantClient(":memory:") yield client await client.close() `@pytest.fixture` def collection_name_sync(sync_client): name = "test_collection_if_not_exists" yield name if sync_client.collection_exists(name): sync_client.delete_collection(name) def test_create_collection_if_not_exists_sync(sync_client, collection_name_sync): result = sync_client.create_collection( collection_name=collection_name_sync, vectors_config=models.VectorParams(size=10, distance=models.Distance.COSINE), if_not_exists=True, ) assert result is True assert sync_client.collection_exists(collection_name_sync) # idempotent result = sync_client.create_collection( collection_name=collection_name_sync, vectors_config=models.VectorParams(size=10, distance=models.Distance.COSINE), if_not_exists=True, ) assert result is TrueAlso applies to: 46-46, 73-73, 91-91, 110-110, 130-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_create_collection_if_not_exists.py` at line 29, Tests call client.delete_collection (and await delete_collection) at the end of each test which will be skipped if an assertion fails; refactor to use pytest fixtures with yield to guarantee cleanup: create fixtures like sync_client (yields QdrantClient(":memory:") and closes it after yield) and async_client (yields AsyncQdrantClient and closes after yield), and create per-test collection_name fixtures (e.g., collection_name_sync / collection_name_async) that yield the collection name and, after yield, check client.collection_exists(name) and delete_collection(name) (or await delete_collection) to ensure deletion even if the test fails; update tests that reference create_collection_if_not_exists to accept these fixtures and remove the trailing manual delete_collection calls.
50-73: Missingawait client.close()forAsyncQdrantClientinstances
AsyncQdrantClientholds underlying async transports/connections. Without closing, tests may produceResourceWarningor unclosed-transport warnings and leave dangling handles in the event loop.♻️ Minimal fix — wrap in try/finally until a fixture is adopted
`@pytest.mark.asyncio` async def test_create_collection_if_not_exists_async(): client = AsyncQdrantClient(":memory:") collection_name = "test_collection_if_not_exists_async" - - result = await client.create_collection(...) - ... - await client.delete_collection(collection_name) + try: + result = await client.create_collection(...) + ... + finally: + await client.delete_collection(collection_name) + await client.close()Also applies to: 77-91, 114-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_create_collection_if_not_exists.py` around lines 50 - 73, The async test creates an AsyncQdrantClient but never closes it, which can leave open transports; wrap the test body that uses AsyncQdrantClient (the client variable in test_create_collection_if_not_exists_async and the other async tests around lines mentioned) in a try/finally and call await client.close() in the finally block (or ensure client.close() is awaited at the end of the test) so underlying connections are properly closed; reference the AsyncQdrantClient instance named client and methods create_collection/collection_exists/delete_collection when locating where to add the finally/await client.close() cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_create_collection_if_not_exists.py`:
- Line 29: Tests call client.delete_collection (and await delete_collection) at
the end of each test which will be skipped if an assertion fails; refactor to
use pytest fixtures with yield to guarantee cleanup: create fixtures like
sync_client (yields QdrantClient(":memory:") and closes it after yield) and
async_client (yields AsyncQdrantClient and closes after yield), and create
per-test collection_name fixtures (e.g., collection_name_sync /
collection_name_async) that yield the collection name and, after yield, check
client.collection_exists(name) and delete_collection(name) (or await
delete_collection) to ensure deletion even if the test fails; update tests that
reference create_collection_if_not_exists to accept these fixtures and remove
the trailing manual delete_collection calls.
- Around line 50-73: The async test creates an AsyncQdrantClient but never
closes it, which can leave open transports; wrap the test body that uses
AsyncQdrantClient (the client variable in
test_create_collection_if_not_exists_async and the other async tests around
lines mentioned) in a try/finally and call await client.close() in the finally
block (or ensure client.close() is awaited at the end of the test) so underlying
connections are properly closed; reference the AsyncQdrantClient instance named
client and methods create_collection/collection_exists/delete_collection when
locating where to add the finally/await client.close() cleanup.
- Use pytest fixtures with yield for sync and async clients - Ensure AsyncQdrantClient.close() is always called - Remove manual delete_collection calls that could be skipped on assertion failure
|
Refactored the tests per review feedback — all tests now use pytest fixtures with |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_create_collection_if_not_exists.py`:
- Around line 42-50: The test test_create_collection_if_not_exists_false_sync
currently relies on the default for if_not_exists and doesn't validate the
explicit kwarg; update the call to sync_client.create_collection in that test to
pass if_not_exists=False explicitly (and do the same for the corresponding async
test around lines 79-87), ensuring you reference the same collection_name and
vectors_config parameters so the test exercises the explicit plumbing of the
if_not_exists argument rather than the default behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7498a7c6-375c-4969-a99d-9c0dacc88e0b
📒 Files selected for processing (1)
tests/test_create_collection_if_not_exists.py
|
friendly bump - let me know if there's any feedback! |
1 participant
Summary
if_not_existsboolean parameter tocreate_collection()methodif_not_exists=True, only creates collection if it doesn't already existQdrantClientandAsyncQdrantClientif_not_exists=False)Motivation
Currently users need to manually check if a collection exists before creating it:
With this change, users can simplify this to:
Test Plan
if_not_exists=Trueif_not_exists=TrueFixes