Skip to content

Conversation

Shades-en
Copy link

What is happening?

When below conditions are met -

  1. I Initialise EmbeddingCache with async_redis_client only.
  2. I then use get method of EmbeddingCache - cache.get(text=text, model_name=model_name)

It defaults to using local redis client instead of the supplied async_redis_client without any warnings. Which in turn results to empty cache always.

It gets worse when we use SemanticCache and use EmbeddingCache with async_redis_client in its vectorizer

Since SemanticCache only accepts synchronous redis_client we may tend to use check method or store method instead of astore or acheck method, which in turn again calls cache.get(text=text, model_name=model_name) and it return EMPTY CACHE

Hence warning logs become neccessary.

What is done?

I have added some warning logs in case if developer supplies async_redis_client but uses get/ mget/set/mset methods.

…ed with get method of embedding cache when async redis client is provided
@bsbodden bsbodden self-assigned this Sep 29, 2025
@bsbodden bsbodden self-requested a review September 29, 2025 18:18
@bsbodden bsbodden changed the title Added warning logs for triaging issues of default local redis instance being used although remote async redis instance is provided feat(cache): add warnings when using sync methods with async-only Redis client Sep 29, 2025
@bsbodden
Copy link
Collaborator

Suggested Improvements

I've reviewed this PR and have some suggestions to improve the warning messages and prevent log spam:

1. Add a class-level warning flag

Add this after line 14 (class definition):

class EmbeddingsCache(BaseCache):
    """Embeddings Cache for storing embedding vectors with exact key matching."""

    _warning_shown: bool = False  # Class-level flag to prevent warning spam

2. Improve warning messages

Replace the warning blocks in all four methods with cleaner, spam-prevented versions:

In get_by_key method (around line 170):

if self._owns_redis_client is False and self._redis_client is None:
    if not EmbeddingsCache._warning_shown:
        logger.warning(
            "EmbeddingsCache initialized with async_redis_client only. "
            "Use async methods (aget_by_key) instead of sync methods (get_by_key)."
        )
        EmbeddingsCache._warning_shown = True

In mget_by_keys method (around line 205):

if self._owns_redis_client is False and self._redis_client is None:
    if not EmbeddingsCache._warning_shown:
        logger.warning(
            "EmbeddingsCache initialized with async_redis_client only. "
            "Use async methods (amget_by_keys) instead of sync methods (mget_by_keys)."
        )
        EmbeddingsCache._warning_shown = True

In set method (around line 286):

if self._owns_redis_client is False and self._redis_client is None:
    if not EmbeddingsCache._warning_shown:
        logger.warning(
            "EmbeddingsCache initialized with async_redis_client only. "
            "Use async methods (aset) instead of sync methods (set)."
        )
        EmbeddingsCache._warning_shown = True

In mset method (around line 336):

if self._owns_redis_client is False and self._redis_client is None:
    if not EmbeddingsCache._warning_shown:
        logger.warning(
            "EmbeddingsCache initialized with async_redis_client only. "
            "Use async methods (amset) instead of sync methods (mset)."
        )
        EmbeddingsCache._warning_shown = True

Benefits of these changes:

  1. Clearer messages - Directly tell users which async method to use
  2. No log spam - Warning only shows once per session
  3. Better DX - More actionable guidance for developers

Test Coverage

Consider adding tests to verify:

  • Warnings appear when using sync methods with async-only client
  • Warning only shows once (no spam)
  • No warnings when using correct client types

I have a complete test file ready if you'd like to add test coverage for this feature.

Copy link
Collaborator

@bsbodden bsbodden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments on the PR conversation, also rebase against main. Thank you!

@bsbodden
Copy link
Collaborator

@Shades-en I'm closing this one and replacing with a rebased version and an extra test #390 - you'll be acknowledge in the release notes, thank you for the contribution!

@bsbodden bsbodden closed this Sep 29, 2025
@Shades-en
Copy link
Author

@Shades-en I'm closing this one and replacing with a rebased version and an extra test #390 - you'll be acknowledge in the release notes, thank you for the contribution!

Thank you @bsbodden sure that would be great! You can acknowledge this account itself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants