-
Notifications
You must be signed in to change notification settings - Fork 57
feat(cache): add warnings when using sync methods with async-only Redis client #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+130
−0
Merged
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
9d2297e
Added warning logs for triaging issues of local redis client being us…
a6b5f50
feat(cache): improve warning messages for async-only client usage
bsbodden 67c009a
fix(tests): resolve event loop closure errors in async Redis client t…
bsbodden 89d26aa
fix: use redis_url fixture in warning tests for CI compatibility
bsbodden 7b7183b
fix: use async_client fixture for async Redis connections in tests
bsbodden 8ad8351
fix: mock Redis client in warning test to prevent connection attempts
bsbodden a2fb8f1
refactor: address Copilot review suggestions
bsbodden bf5d82e
Update redisvl/extensions/cache/embeddings/embeddings.py
bsbodden File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
"""Test warning behavior when using sync methods with async-only client.""" | ||
|
||
import logging | ||
from unittest.mock import patch | ||
|
||
import pytest | ||
from redis import Redis | ||
|
||
from redisvl.extensions.cache.embeddings import EmbeddingsCache | ||
|
||
|
||
@pytest.fixture(autouse=True) | ||
def reset_warning_flag(): | ||
"""Reset the warning flag before each test to ensure test isolation.""" | ||
EmbeddingsCache._warning_shown = False | ||
yield | ||
# Optionally reset after test as well for cleanup | ||
EmbeddingsCache._warning_shown = False | ||
|
||
|
||
@pytest.mark.asyncio | ||
async def test_sync_methods_warn_with_async_only_client(async_client, caplog): | ||
"""Test that sync methods warn when only async client is provided.""" | ||
# Initialize EmbeddingsCache with only async_redis_client | ||
cache = EmbeddingsCache(name="test_cache", async_redis_client=async_client) | ||
|
||
# Mock _get_redis_client to prevent actual connection attempt | ||
with patch.object(cache, "_get_redis_client") as mock_get_client: | ||
# Mock the Redis client methods that would be called | ||
mock_client = mock_get_client.return_value | ||
mock_client.hgetall.return_value = {} # Empty result for get_by_key | ||
mock_client.hset.return_value = 1 # Success for set | ||
|
||
# Capture log warnings | ||
with caplog.at_level(logging.WARNING): | ||
# First sync method call should warn | ||
_ = cache.get_by_key("test_key") | ||
|
||
# Check warning was logged | ||
assert len(caplog.records) == 1 | ||
assert ( | ||
"initialized with async_redis_client only" in caplog.records[0].message | ||
) | ||
assert "Use async methods" in caplog.records[0].message | ||
|
||
# Clear captured logs | ||
caplog.clear() | ||
|
||
# Second sync method call should NOT warn (flag prevents spam) | ||
_ = cache.set(text="test", model_name="model", embedding=[0.1, 0.2]) | ||
|
||
# Should not have logged another warning | ||
assert len(caplog.records) == 0 | ||
|
||
|
||
def test_no_warning_with_sync_client(redis_url): | ||
"""Test that no warning is shown when sync client is provided.""" | ||
# Create sync redis client from redis_url | ||
sync_client = Redis.from_url(redis_url) | ||
|
||
try: | ||
# Initialize EmbeddingsCache with sync_redis_client | ||
cache = EmbeddingsCache(name="test_cache", redis_client=sync_client) | ||
|
||
with patch("redisvl.utils.log.get_logger") as mock_logger: | ||
# Sync methods should not warn | ||
_ = cache.get_by_key("test_key") | ||
_ = cache.set(text="test", model_name="model", embedding=[0.1, 0.2]) | ||
|
||
# No warnings should have been logged | ||
mock_logger.return_value.warning.assert_not_called() | ||
finally: | ||
sync_client.close() | ||
|
||
|
||
@pytest.mark.asyncio | ||
async def test_async_methods_no_warning(async_client): | ||
"""Test that async methods don't trigger warnings.""" | ||
# Initialize EmbeddingsCache with only async_redis_client | ||
cache = EmbeddingsCache(name="test_cache", async_redis_client=async_client) | ||
|
||
with patch("redisvl.utils.log.get_logger") as mock_logger: | ||
# Async methods should not warn | ||
_ = await cache.aget_by_key("test_key") | ||
_ = await cache.aset(text="test", model_name="model", embedding=[0.1, 0.2]) | ||
|
||
# No warnings should have been logged | ||
mock_logger.return_value.warning.assert_not_called() |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.