Conversation
Add CacheConfig with strategy="auto" for BedrockModel to automatically inject cache points at the end of assistant messages in multi-turn conversations. - Add CacheConfig dataclass in model.py with strategy field - Add supports_caching property to check Claude model compatibility - Implement _inject_cache_point() for automatic cache point management - Export CacheConfig from models/__init__.py Closes strands-agents#1432
- Add warning when cache_config enabled but model doesn't support caching - Make supports_caching private (_supports_caching) - Fix log formatting to follow style guide - Clean up tests and imports
|
/strands review |
| cache_config = self.config.get("cache_config") | ||
| if cache_config and cache_config.strategy == "auto": | ||
| if self._supports_caching: | ||
| self._inject_cache_point(messages) |
There was a problem hiding this comment.
Issue: _inject_cache_point mutates the original messages list in-place, which can lead to unexpected side effects on agent.messages.
Suggestion: Make a deep copy of the messages before injecting cache points:
import copy
# In _format_bedrock_messages:
if cache_config and cache_config.strategy == "auto":
if self._supports_caching:
messages = copy.deepcopy(messages) # Work on a copy
self._inject_cache_point(messages)This ensures that agent.messages remains unmodified and cache points are only in the request payload sent to Bedrock.
| Returns True for Claude models on Bedrock. | ||
| """ | ||
| model_id = self.config.get("model_id", "").lower() | ||
| return "claude" in model_id or "anthropic" in model_id |
There was a problem hiding this comment.
Suggestion: Consider making this check more robust. Currently it uses string matching which could have edge cases:
- A model ID containing "claude" in a different context
- Future Anthropic models with different naming
Consider adding a comment explaining the heuristic, or checking against a known list of supported model patterns.
| ) | ||
|
|
||
| # Simulate a mock BedrockModel with cache_config | ||
| mock_model.get_config = unittest.mock.MagicMock( |
There was a problem hiding this comment.
Issue: This test uses mock_model which doesn't have the _inject_cache_point method, so it doesn't actually test whether BedrockModel mutates messages.
Suggestion: Move this test to tests/strands/models/test_bedrock.py and use a real BedrockModel instance to verify the mutation behavior. For example:
def test_format_bedrock_messages_does_not_mutate_original(bedrock_client):
"""Test that _format_bedrock_messages doesn't mutate the original messages."""
model = BedrockModel(
model_id="us.anthropic.claude-sonnet-4-20250514-v1:0",
cache_config=CacheConfig(strategy="auto")
)
messages = [
{"role": "user", "content": [{"text": "Hello"}]},
{"role": "assistant", "content": [{"text": "Hi there!"}]},
]
original_content_len = len(messages[1]["content"])
model._format_bedrock_messages(messages)
# Original messages should be unchanged
assert len(messages[1]["content"]) == original_content_len
Review SummaryAssessment: Request Changes Key ThemesThe implementation approach is well thought out - automatically moving cache points to the last assistant message is a clean way to maximize cache hits. The code is well-structured and follows the repository's coding patterns. Critical Issue:
Testing Gaps:
Minor Suggestions:
Nice work on the feature design! The cache point auto-injection is a useful addition. Once the mutation issue is addressed, this will be a solid contribution. 🚀 |
Summary
CacheConfigwithstrategy="auto"for automatic prompt caching inBedrockModelUsage
Test plan