Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@joein has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new utility function, ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
qdrant_client/embed/utils.py (1)
71-79: Consider renaming function to reflect its generic file capability.The function implementation is correct and handles file operations properly. However, the function name
to_base64with parameterimage_pathsuggests it's specifically for images, but the implementation works for any file type.Consider renaming the parameter to
file_pathto better reflect its generic capability:-def to_base64(image_path: Union[str, Path]) -> str: - """Convert a file path to a base64 encoded string.""" - path = Path(image_path) +def to_base64(file_path: Union[str, Path]) -> str: + """Convert a file path to a base64 encoded string.""" + path = Path(file_path)The error handling and base64 encoding logic are implemented correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
qdrant_client/embed/utils.py(2 hunks)tests/embed_tests/test_utils.py(1 hunks)tests/utils.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
🔇 Additional comments (4)
tests/utils.py (1)
3-6: LGTM! Clean addition of test path constant.The addition of the
Pathimport andTESTS_PATHconstant follows good practices for test utilities. This provides a reliable way to reference test data files.qdrant_client/embed/utils.py (1)
1-3: LGTM! Necessary imports added correctly.The imports are appropriate for the new base64 encoding functionality.
tests/embed_tests/test_utils.py (2)
1-11: LGTM! Well-structured test setup.The imports and test data path setup are correct and follow good testing practices.
12-27: Verify test data file exists before running tests.The test logic is comprehensive and correctly validates both successful encoding and error handling. However, the test assumes the existence of
tests/embed_tests/misc/image.jpegwithout verification.Consider adding a check to ensure the test data file exists:
def test_image_path_to_b64(): # Test with a valid image file image_path = Path(EMBED_TESTS_DATA / "image.jpeg") + assert image_path.exists(), f"Test image file not found: {image_path}" original_bytes = image_path.read_bytes()The test correctly validates that:
- The function returns a string
- The base64 encoding is reversible (decoded bytes match original)
- FileNotFoundError is raised for non-existent files
Please verify that the test image file exists at the expected location:
#!/bin/bash # Check if the test image file exists fd -t f "image.jpeg" tests/embed_tests/misc/
| return converted_paths | ||
|
|
||
|
|
||
| def to_base64(image_path: Union[str, Path]) -> str: |
There was a problem hiding this comment.
Maybe a different name for this function?
Is it agnostic to file type? If yes, it should be image_path -> file_path, and read_base64
If it is specific to iamges, I would use something like base64_image
There was a problem hiding this comment.
It is agnostic to file type, though, I don't think we expect it to be used in qdrant-client with anything but image files
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_remove_inference.py (3)
64-66: Consider more specific exception handling.The broad
except Exceptionclause catches all exceptions without being specific about expected error types. For test code, consider either:
- Catching specific expected exceptions (e.g., connection errors, API errors)
- Removing the try-catch block to let exceptions propagate naturally for better test debugging
- try: - client.upsert( + client.upsert( collection_name=collection_name, points=[ models.PointStruct( id=1, vector=models.Image( image=image_url, model=model_name, ), ), - # models.PointStruct( - # id=2, - # vector=models.Image( - # image=read_base64(image_path), - # model=model_name, - # ), - # ), - ] - ) - except Exception as e: - print(e) - raise e + ] + )
55-61: Consider enabling the local file comparison functionality.The commented-out code suggests the test is intended to compare remote URL inference with local file inference using
read_base64. The test comment on line 24 reinforces this intention.If the
read_base64functionality is ready, consider uncommenting this code to complete the test's intended purpose. If it's not ready, consider adding a TODO comment explaining the current status.
16-16: Consider clarifying the test name.The function name
test_remove_inference_imagemight be confusing as the test appears to be testing image embedding functionality rather than "remove inference." Consider renaming to something more descriptive liketest_image_embedding_inferenceortest_remote_image_inference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_remove_inference.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/test_remove_inference.py
6-6: qdrant_client.embed.utils.read_base64 imported but unused
Remove unused import: qdrant_client.embed.utils.read_base64
(F401)
🪛 Flake8 (7.2.0)
tests/test_remove_inference.py
[error] 6-6: 'qdrant_client.embed.utils.read_base64' imported but unused
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
🔇 Additional comments (2)
tests/test_remove_inference.py (2)
6-6: Clarify the unused import or enable the functionality.The
read_base64import is flagged as unused by static analysis tools, but the commented-out code on lines 55-61 shows its intended use. Consider either:
- Uncommenting the code to test local file functionality alongside remote URL inference
- Removing the import temporarily if the feature isn't ready
- Adding a comment explaining why the import is present but unused
1-67: Well-structured integration test with room for improvement.The test follows good practices with:
- Appropriate skip marker for integration tests requiring external dependencies
- Environment variable configuration
- Proper collection cleanup and setup
The main areas for improvement are the cross-platform compatibility issue and completing the intended comparison functionality between remote URL and local file inference.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_remove_inference.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
🔇 Additional comments (1)
tests/test_remove_inference.py (1)
1-11: LGTM! Clean imports and proper environment variable usage.The imports are well-organized and include all necessary dependencies for the test functionality. Using environment variables for configuration is a good practice for test flexibility.
| client.upsert( | ||
| collection_name=collection_name, | ||
| points=[ | ||
| models.PointStruct( | ||
| id=1, | ||
| vector=models.Image( | ||
| image=image_url, | ||
| model=model_name, | ||
| ), | ||
| ), | ||
| models.PointStruct( | ||
| id=2, | ||
| vector=models.Image( | ||
| image=read_base64(image_path), | ||
| model=model_name, | ||
| ), | ||
| ), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test assertions to verify the functionality.
The test function performs setup and data insertion but lacks assertions to verify the expected behavior. It should validate that:
- The upsert operations were successful
- The embeddings from URL and local file are comparable
- The
read_base64utility function works correctly
Add verification logic:
client.upsert(
collection_name=collection_name,
points=[
models.PointStruct(
id=1,
vector=models.Image(
image=image_url,
model=model_name,
),
),
models.PointStruct(
id=2,
vector=models.Image(
image=read_base64(image_path),
model=model_name,
),
),
]
)
+
+ # Verify points were inserted successfully
+ collection_info = client.get_collection(collection_name)
+ assert collection_info.points_count == 2
+
+ # Verify both points can be retrieved
+ points = client.retrieve(collection_name, ids=[1, 2])
+ assert len(points) == 2
+
+ # Optionally verify embeddings are similar (since they're the same image)
+ search_results = client.search(
+ collection_name=collection_name,
+ query_vector=points[0].vector,
+ limit=2
+ )
+ assert len(search_results) == 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client.upsert( | |
| collection_name=collection_name, | |
| points=[ | |
| models.PointStruct( | |
| id=1, | |
| vector=models.Image( | |
| image=image_url, | |
| model=model_name, | |
| ), | |
| ), | |
| models.PointStruct( | |
| id=2, | |
| vector=models.Image( | |
| image=read_base64(image_path), | |
| model=model_name, | |
| ), | |
| ), | |
| ] | |
| ) | |
| client.upsert( | |
| collection_name=collection_name, | |
| points=[ | |
| models.PointStruct( | |
| id=1, | |
| vector=models.Image( | |
| image=image_url, | |
| model=model_name, | |
| ), | |
| ), | |
| models.PointStruct( | |
| id=2, | |
| vector=models.Image( | |
| image=read_base64(image_path), | |
| model=model_name, | |
| ), | |
| ), | |
| ] | |
| ) | |
| # Verify points were inserted successfully | |
| collection_info = client.get_collection(collection_name) | |
| assert collection_info.points_count == 2 | |
| # Verify both points can be retrieved | |
| points = client.retrieve(collection_name, ids=[1, 2]) | |
| assert len(points) == 2 | |
| # Optionally verify embeddings are similar (since they're the same image) | |
| search_results = client.search( | |
| collection_name=collection_name, | |
| query_vector=points[0].vector, | |
| limit=2 | |
| ) | |
| assert len(search_results) == 2 |
🤖 Prompt for AI Agents
In tests/test_remove_inference.py between lines 47 and 65, the test inserts data
but lacks assertions to verify outcomes. Add assertions to confirm the upsert
operation succeeded by checking the response status or result. Then retrieve the
inserted points and assert that embeddings from the URL and local file are
similar within an acceptable tolerance. Also, add an assertion to verify that
the read_base64 function returns the expected base64 string for the local image
file.
| with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as tmp_file: | ||
| image_path = tmp_file.name | ||
|
|
||
| with httpx.Client() as httpx_client: | ||
| response = httpx_client.get(image_url) | ||
| with open(image_path, "wb") as f: | ||
| f.write(response.content) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for temporary file and error handling.
While the cross-platform temporary file approach is good, the test is missing:
- Cleanup code to delete the temporary file after test completion
- Error handling for HTTP requests and file operations
Add cleanup and error handling:
+import os
+
def test_remove_inference_image():
+ image_path = None
+ try:
client = QdrantClient(url=QDRANT_URL, api_key=QDRANT_API_KEY, cloud_inference=True)
collection_name = "image_embeddings"
model_name = "Qdrant/clip-ViT-B-32-vision"
image_url = "https://qdrant.tech/example.png"
# Compare inference of image exposed via url and local file
# So download image to local file and compare results
with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as tmp_file:
image_path = tmp_file.name
with httpx.Client() as httpx_client:
response = httpx_client.get(image_url)
+ response.raise_for_status() # Raise exception for bad status codes
with open(image_path, "wb") as f:
f.write(response.content)
+ finally:
+ # Clean up temporary file
+ if image_path and os.path.exists(image_path):
+ os.unlink(image_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as tmp_file: | |
| image_path = tmp_file.name | |
| with httpx.Client() as httpx_client: | |
| response = httpx_client.get(image_url) | |
| with open(image_path, "wb") as f: | |
| f.write(response.content) | |
| import os | |
| def test_remove_inference_image(): | |
| image_path = None | |
| try: | |
| client = QdrantClient(url=QDRANT_URL, api_key=QDRANT_API_KEY, cloud_inference=True) | |
| collection_name = "image_embeddings" | |
| model_name = "Qdrant/clip-ViT-B-32-vision" | |
| image_url = "https://qdrant.tech/example.png" | |
| # Compare inference of image exposed via url and local file | |
| # So download image to local file and compare results | |
| with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as tmp_file: | |
| image_path = tmp_file.name | |
| with httpx.Client() as httpx_client: | |
| response = httpx_client.get(image_url) | |
| response.raise_for_status() # Raise exception for bad status codes | |
| with open(image_path, "wb") as f: | |
| f.write(response.content) | |
| finally: | |
| # Clean up temporary file | |
| if image_path and os.path.exists(image_path): | |
| os.unlink(image_path) |
🤖 Prompt for AI Agents
In tests/test_remove_inference.py around lines 28 to 35, the temporary file
created for the image is not deleted after the test, and there is no error
handling for the HTTP request or file writing. To fix this, add a try-finally
block around the HTTP request and file write operations to ensure the temporary
file is deleted in the finally clause. Also, add exception handling for the HTTP
request and file operations to catch and handle potential errors gracefully.
* new: add b64 encoding function * tests: remove garbage * refactor base64 function name and params * manual test for remove iamge inference with base64 * use tempfile * fix: rename remove_inference->remote_inference --------- Co-authored-by: generall <[email protected]>
2 participants
No description provided.