feat(cache): add warnings when using sync methods with async-only Redis client #391
Conversation
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
There was a problem hiding this comment.
Pull Request Overview
This PR adds warning functionality to the EmbeddingsCache class to alert users when they attempt to use synchronous methods while only having an async Redis client configured. The warnings help guide users toward the appropriate async methods when sync operations may fail or behave unexpectedly.
- Adds a class-level flag to prevent warning spam after the first occurrence
- Implements warning logic in sync methods when only async client is available
- Includes comprehensive test coverage for the warning behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| redisvl/extensions/cache/embeddings/embeddings.py | Adds warning logic to sync methods (get_by_key, mget_by_keys, set, mset) when only async client is configured |
| tests/integration/test_embedcache_warnings.py | Comprehensive test suite covering warning behavior in different client configurations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…ed with get method of embedding cache when async redis client is provided
- Add concise warning messages when sync methods are called with async-only client - Implement warning flag to prevent log spam (show warning only once per session) - Add comprehensive test coverage for warning behavior - Improve developer experience by providing clear guidance on method usage The warnings now clearly indicate which async methods should be used instead of sync methods when EmbeddingsCache is initialized with only an async client. This helps prevent confusion when developers unintentionally use different Redis connections than expected.
…ests - Convert sync test functions to async when using async Redis clients - Use _get_aredis_connection() for proper async client creation - Add proper try/finally blocks to ensure client cleanup - Fix 'Event loop is closed' errors in CI The issue was that async Redis clients were being created in sync test functions without proper event loop management. This caused cleanup issues when the test finished.
- Replace direct Redis connection with redis_url fixture - Use AsyncRedis.from_url() instead of RedisConnectionFactory - Add proper try/finally blocks for client cleanup - Follow established test patterns from test_embedcache.py This fixes 'Event loop is closed' errors in CI by using the same Docker container-based Redis connection that other tests use.
- Replace manual AsyncRedis.from_url() with async_client fixture - Remove unnecessary AsyncRedis import - Remove manual client cleanup (handled by fixture) - Follows the pattern used by all other async tests in the codebase This properly uses the conftest async_client fixture which handles the Redis connection with proper context manager cleanup.
The test was failing in CI because sync methods were still trying to create a Redis connection to localhost:6379 after logging the warning. Now we mock _get_redis_client() to prevent actual connection attempts while still testing the warning functionality. This allows the test to verify warning behavior without requiring a Redis connection for the sync methods that shouldn't be used anyway.
- Extract duplicated condition check into _should_warn_for_async_only() helper method - Add pytest autouse fixture to reset warning flag before each test - Improves code maintainability and test isolation Co-authored-by: GitHub Copilot <[email protected]>
bsbodden
force-pushed
the
bsb/sync-on-async-warnings
branch
from
5d9df94 to
a2fb8f1
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
bsbodden
added a commit
that referenced
this pull request
Sep 30, 2025
…is client (#391) Co-authored-by: owais <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.