feat(valkey): shared cache config + ValkeyCache for A2A and file uploads#5700
Open
MatthiasHowellYopp wants to merge 2 commits into
Open
feat(valkey): shared cache config + ValkeyCache for A2A and file uploads#5700MatthiasHowellYopp wants to merge 2 commits into
MatthiasHowellYopp wants to merge 2 commits into
Conversation
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
3 times, most recently
from
79047cb to
c459701
Compare
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
c459701 to
a4ba47b
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Valkey-GLIDE support and a CacheBackend abstraction, centralizes cache config, implements ValkeyCache, serializes CachedUpload, refactors UploadCache and task cancellation to use pluggable backends, updates agent cache init, and adds tests and an optional valkey dependency. ChangesValkey Cache Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Contributor
Author
|
@greysonlalonde Hi, hoping someone can look at the 4 valkey PR's this week, tks. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/crewai-files/src/crewai_files/cache/upload_cache.py (2)
156-160: 💤 Low valueUse lazy % formatting for logging.
Minor: Consider using lazy formatting for the warning log to avoid string interpolation when the log level is disabled.
♻️ Suggested change
try: return CachedUpload.from_dict(data) except (KeyError, ValueError) as e: - logger.warning(f"Failed to deserialize cached upload: {e}") + logger.warning("Failed to deserialize cached upload: %s", e) return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai-files/src/crewai_files/cache/upload_cache.py` around lines 156 - 160, The logger.warning call in upload_cache.py uses f-string interpolation (logger.warning(f"Failed to deserialize cached upload: {e}")) which eagerly formats the message; change it to use lazy logging formatting (logger.warning("Failed to deserialize cached upload: %s", e)) in the except block that handles KeyError/ValueError around CachedUpload.from_dict so the string interpolation is deferred when the warning level is disabled.
238-253: ⚖️ Poor tradeoffProvider key tracking is instance-local, limiting distributed cleanup.
The
_provider_keysdictionary tracks keys locally within eachUploadCacheinstance. In a distributed Valkey setup with multiple application instances, each instance only knows about keys it created. This meansaclear(),aclear_expired(), andaget_all_for_provider()will only operate on keys known to that specific instance.This is an inherent limitation of the current design. Consider documenting this behavior or, for future enhancement, implementing key discovery via Valkey's
SCANcommand with the namespace prefix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai-files/src/crewai_files/cache/upload_cache.py` around lines 238 - 253, The provider-key tracking via _provider_keys (managed by _track_key and _untrack_key) is instance-local, so aclear, aclear_expired and aget_all_for_provider only affect keys known to that UploadCache instance; update the UploadCache class and the docstrings for _track_key, _untrack_key, aclear, aclear_expired, and aget_all_for_provider to explicitly document this limitation and mention a future enhancement: using Valkey's SCAN with the namespace prefix to discover keys across distributed instances for global cleanup.lib/crewai/src/crewai/memory/storage/valkey_cache.py (1)
126-130: 💤 Low valueUse lazy % formatting for logging.
The f-string in the warning log is evaluated even if the log level is disabled. Consider using lazy formatting for better performance.
♻️ Suggested change
try: return json.loads(value) except json.JSONDecodeError: - _logger.warning(f"Failed to deserialize cached value for key: {key}") + _logger.warning("Failed to deserialize cached value for key: %s", key) return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/memory/storage/valkey_cache.py` around lines 126 - 130, The warning log in valkey_cache.py currently uses an f-string which is evaluated eagerly; update the except block that handles json.JSONDecodeError (the one returning None after attempting json.loads(value)) to use lazy logging with _logger.warning("Failed to deserialize cached value for key: %s", key) so the interpolated key is only formatted when the warning level is enabled; leave the rest of the try/except logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/memory/storage/valkey_cache.py`:
- Around line 145-160: json.dumps(value) can raise TypeError for
non-JSON-serializable objects; wrap the serialization in a try/except around the
json.dumps(value) call (inside the method using _get_client, ttl_to_use and
_default_ttl) and handle failures by either (a) falling back to a safe
serializer like json.dumps(value, default=str) or (b) raising a clearer error
including context (e.g., key and value type) so callers don't get an unhelpful
stacktrace; ensure the rest of the logic still uses the serialized variable for
client.set and that the TTL logic (ttl_to_use) remains unchanged.
---
Nitpick comments:
In `@lib/crewai-files/src/crewai_files/cache/upload_cache.py`:
- Around line 156-160: The logger.warning call in upload_cache.py uses f-string
interpolation (logger.warning(f"Failed to deserialize cached upload: {e}"))
which eagerly formats the message; change it to use lazy logging formatting
(logger.warning("Failed to deserialize cached upload: %s", e)) in the except
block that handles KeyError/ValueError around CachedUpload.from_dict so the
string interpolation is deferred when the warning level is disabled.
- Around line 238-253: The provider-key tracking via _provider_keys (managed by
_track_key and _untrack_key) is instance-local, so aclear, aclear_expired and
aget_all_for_provider only affect keys known to that UploadCache instance;
update the UploadCache class and the docstrings for _track_key, _untrack_key,
aclear, aclear_expired, and aget_all_for_provider to explicitly document this
limitation and mention a future enhancement: using Valkey's SCAN with the
namespace prefix to discover keys across distributed instances for global
cleanup.
In `@lib/crewai/src/crewai/memory/storage/valkey_cache.py`:
- Around line 126-130: The warning log in valkey_cache.py currently uses an
f-string which is evaluated eagerly; update the except block that handles
json.JSONDecodeError (the one returning None after attempting json.loads(value))
to use lazy logging with _logger.warning("Failed to deserialize cached value for
key: %s", key) so the interpolated key is only formatted when the warning level
is enabled; leave the rest of the try/except logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 217f16f8-f68f-4fc9-a12b-b58fc31423ac
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
lib/crewai-files/src/crewai_files/cache/upload_cache.pylib/crewai/pyproject.tomllib/crewai/src/crewai/a2a/utils/agent_card.pylib/crewai/src/crewai/a2a/utils/task.pylib/crewai/src/crewai/memory/storage/valkey_cache.pylib/crewai/src/crewai/utilities/cache_config.pylib/crewai/tests/memory/storage/test_valkey_cache.pylib/crewai/tests/utilities/test_cache_config.py
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
a4ba47b to
d745a45
Compare
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
2 times, most recently
from
540e9ba to
fba2c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai/src/crewai/memory/storage/valkey_cache.py (1)
1-199:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix formatting to unblock CI.
uv run ruff format --check lib/is failing in pipeline; please run formatter on this file (and any other changed files) before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/memory/storage/valkey_cache.py` around lines 1 - 199, CI is failing due to formatting issues; run the project's formatter (ruff format as used in CI) on this file and any other modified files to satisfy "uv run ruff format --check lib/". Reformat the module containing the ValkeyCache class (and its methods _get_client, get, set, delete, exists, close) so import ordering, spacing, and line breaks conform to the formatter, then re-run the ruff format check and commit the formatted changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/memory/storage/valkey_cache.py`:
- Around line 159-169: The current logic in valkey_cache.py computes ttl_to_use
from ttl or self._default_ttl and treats negative values (e.g., ttl == -1) as
"no-expiration", which can hide caller bugs; update the code around ttl_to_use
to explicitly reject negative TTLs by raising a ValueError (or similar) when
ttl_to_use < 0, and only call client.set(..., expiry=ExpirySet(ExpiryType.SEC,
ttl_to_use)) when ttl_to_use > 0 otherwise call client.set(key, serialized) for
None/zero; adjust references: ttl_to_use, _default_ttl, client.set, ExpirySet,
ExpiryType.SEC.
- Around line 57-109: Concurrent callers can race creating multiple GlideClient
instances because _get_client checks self._client without synchronization; add
an asyncio.Lock (e.g., self._client_lock created in __init__) and wrap the lazy
initialization in _get_client with "async with self._client_lock:" then recheck
"if self._client is None" inside the lock before calling GlideClient.create (and
keep the existing timeout/error handling), so only one coroutine performs
initialization and others await the same client.
---
Outside diff comments:
In `@lib/crewai/src/crewai/memory/storage/valkey_cache.py`:
- Around line 1-199: CI is failing due to formatting issues; run the project's
formatter (ruff format as used in CI) on this file and any other modified files
to satisfy "uv run ruff format --check lib/". Reformat the module containing the
ValkeyCache class (and its methods _get_client, get, set, delete, exists, close)
so import ordering, spacing, and line breaks conform to the formatter, then
re-run the ruff format check and commit the formatted changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f6f8c3c-37fa-4aa1-835e-7a4d2eab1b56
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
lib/crewai-files/src/crewai_files/cache/upload_cache.pylib/crewai/pyproject.tomllib/crewai/src/crewai/a2a/utils/agent_card.pylib/crewai/src/crewai/a2a/utils/task.pylib/crewai/src/crewai/memory/storage/valkey_cache.pylib/crewai/src/crewai/utilities/cache_config.pylib/crewai/tests/memory/storage/test_valkey_cache.pylib/crewai/tests/utilities/test_cache_config.py
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/crewai/pyproject.toml
- lib/crewai/src/crewai/a2a/utils/agent_card.py
- lib/crewai/tests/utilities/test_cache_config.py
- lib/crewai/src/crewai/utilities/cache_config.py
- lib/crewai/tests/memory/storage/test_valkey_cache.py
- lib/crewai-files/src/crewai_files/cache/upload_cache.py
- lib/crewai/src/crewai/a2a/utils/task.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/pyproject.toml`:
- Around line 113-115: The dependency entry for valkey currently uses an
open-ended constraint ("valkey-glide>=1.3.0"); update the valkey list to pin to
the maintained v2.x series by changing the requirement to a bounded range such
as "valkey-glide>=2.0.0,<3.0.0" so future v3.x breaking changes are avoided
(edit the valkey array entry in pyproject.toml and replace the existing
"valkey-glide>=1.3.0" line).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dba85f4-bcbc-4fd7-84f0-7cb1b19bd2ee
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
lib/crewai-files/src/crewai_files/cache/upload_cache.pylib/crewai/pyproject.tomllib/crewai/src/crewai/a2a/utils/agent_card.pylib/crewai/src/crewai/a2a/utils/task.pylib/crewai/src/crewai/memory/storage/valkey_cache.pylib/crewai/src/crewai/utilities/cache_config.pylib/crewai/tests/memory/storage/test_valkey_cache.pylib/crewai/tests/utilities/test_cache_config.py
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/crewai/src/crewai/a2a/utils/agent_card.py
- lib/crewai/tests/memory/storage/test_valkey_cache.py
- lib/crewai/src/crewai/a2a/utils/task.py
- lib/crewai/tests/utilities/test_cache_config.py
- lib/crewai/src/crewai/memory/storage/valkey_cache.py
- lib/crewai-files/src/crewai_files/cache/upload_cache.py
- lib/crewai/src/crewai/utilities/cache_config.py
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
fba2c17 to
81f403b
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai-files/src/crewai_files/cache/upload_cache.py`:
- Around line 165-167: The delete wrapper always returns True which incorrectly
signals success for non-existent keys; change upload_cache.delete to detect
whether the key actually existed before/after deletion and return that boolean.
Specifically, in delete(self, key: str) call the underlying cache in a way that
determines existence (e.g. check await self._cache.exists(key) or await
self._cache.contains(key) before deleting, or use the boolean return value of
await self._cache.delete(key) if present) and return that real result instead of
unconditionally True; update the code in upload_cache.delete to handle both
ValkeyCache (void delete) and caches that return bool by using existence check
when delete returns None.
- Around line 208-224: The Valkey branch in _create_backend drops the namespace,
causing cross-namespace key collisions; update the ValkeyCacheBackend
construction in _create_backend to pass the namespace through (e.g., include
namespace=namespace or the appropriate key_prefix/namespace parameter accepted
by ValkeyCacheBackend) alongside host/port/db/password/default_ttl so each
UploadCache instance is isolated by namespace.
In `@lib/crewai/src/crewai/memory/storage/valkey_cache.py`:
- Around line 163-174: In the set() method of valkey_cache.py, validate ttl and
JSON-serialize the value before calling self._get_client() so invalid input
raises the documented TypeError/ValueError without opening a Valkey connection;
specifically, move the json.dumps(value) try/except block and the ttl_to_use/ttl
< 0 check to run prior to the call to client = await self._get_client(),
preserving the same exception types and messages raised by the existing
TypeError/ValueError paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01a6c7d7-b80b-4e7a-a7d1-a92d539f95b0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
lib/crewai-files/src/crewai_files/cache/upload_cache.pylib/crewai/pyproject.tomllib/crewai/src/crewai/a2a/utils/agent_card.pylib/crewai/src/crewai/a2a/utils/task.pylib/crewai/src/crewai/memory/storage/valkey_cache.pylib/crewai/src/crewai/utilities/cache_config.pylib/crewai/tests/memory/storage/test_valkey_cache.pylib/crewai/tests/utilities/test_cache_config.py
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/crewai/tests/utilities/test_cache_config.py
- lib/crewai/src/crewai/utilities/cache_config.py
- lib/crewai/src/crewai/a2a/utils/agent_card.py
- lib/crewai/src/crewai/a2a/utils/task.py
- lib/crewai/tests/memory/storage/test_valkey_cache.py
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
81f403b to
c4804a1
Compare
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
2 times, most recently
from
0cbd87a to
8901835
Compare
Contributor
Author
|
@greysonlalonde hoping I can get a review on this. |
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
8901835 to
1a2bdf0
Compare
Contributor
Reply appreciate the patience here, thanks! Will start getting to these today |
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
6 times, most recently
from
752be6f to
20f7ee0
Compare
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
20f7ee0 to
9eabc7f
Compare
Contributor
Author
|
@greysonlalonde Looks like you had a busy week last week, hoping I get lucky this week, fingers crossed. |
|
Before this merges, please add Example: cfg = GlideClientConfiguration(
[NodeAddress(host, port)],
client_name="crewai_valkey",
...
)This is a one-liner addition to the config builder. See valkey-glide docs for reference. |
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
9eabc7f to
c207f49
Compare
Extract duplicated Redis URL parsing into a shared cache_config utility. Introduce ValkeyCache as a lightweight async key/value cache using valkey-glide. Wire it into A2A task handling, agent card caching, and file upload caching. Part 1/4 of Valkey storage implementation.
Sets CLIENT SETNAME to 'crewai_valkey' so connections are identifiable in CLIENT LIST and monitoring tools (Valkey Admin, CloudWatch). Signed-off-by: Matthias Howell <matthias.howell@improving.com>
MatthiasHowellYopp
force-pushed
the
feat/valkey-1-cache-config
branch
from
76e1d6d to
1fcab50
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description:
Part 1/4 of adding Valkey as a storage backend for CrewAI. This PR lays the caching foundation that the vector storage implementation will build on.
What changed:
cache_config.py (new) — Three small helpers: parse_cache_url() reads VALKEY_URL or REDIS_URL from the environment, get_aiocache_config() builds an aiocache config dict from it, and use_valkey_cache() returns whether the Valkey path is active. Replaces the inline _parse_redis_url() that lived in task.py.
valkey_cache.py (new) — A thin async get/set/delete/exists wrapper around valkey-glide. JSON serialization, optional TTL, lazy client initialization. This is the simple key/value cache; vector storage comes in part 4.
task.py — Replaced inline URL parsing and module-level caches.set_config() with the shared cache_config helpers. Task cancellation polling now uses ValkeyCache when VALKEY_URL is set, falling back to aiocache otherwise.
agent_card.py — Added lazy aiocache configuration via get_aiocache_config() so the cache backend is configured before first use rather than at import time.
upload_cache.py — Refactored to optionally use ValkeyCache as its backend when VALKEY_URL is set, with JSON-based serialization instead of pickle.
pyproject.toml — Added valkey-glide>=1.3.0 as an optional [valkey] extra.
Testing:
test_cache_config.py (11 tests) — URL parsing, priority, defaults, aiocache config generation
test_valkey_cache.py (27 tests) — get/set/delete/exists, TTL, JSON serialization, connection management, edge cases
Existing test_agent_card.py, test_task.py, and test_upload_cache.py cover the refactored paths
Summary by CodeRabbit
New Features
Tests