From 6827aef5e6e332119f5c2d1b12e312da78bcc7e2 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Thu, 27 Feb 2025 14:00:23 +0000 Subject: [PATCH] Address code review suggestions for PR #2243: Add timestamp validation, logging, configurable time decay, and edge case testing Co-Authored-By: Joe Moura --- .../memory/short_term/short_term_memory.py | 20 +++++++++ .../short_term/short_term_memory_item.py | 2 + src/crewai/memory/storage/rag_storage.py | 20 ++++++++- tests/memory/test_memory_topic_changes.py | 43 +++++++++++++++++++ 4 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/crewai/memory/short_term/short_term_memory.py b/src/crewai/memory/short_term/short_term_memory.py index 37afdd5ed..70bfdb2d1 100644 --- a/src/crewai/memory/short_term/short_term_memory.py +++ b/src/crewai/memory/short_term/short_term_memory.py @@ -1,3 +1,4 @@ +from datetime import datetime from typing import Any, Dict, Optional from pydantic import PrivateAttr @@ -52,7 +53,26 @@ class ShortTermMemory(Memory): metadata: Optional[Dict[str, Any]] = None, agent: Optional[str] = None, ) -> None: + """ + Save a memory item to the storage. + + Args: + value: The data to save. + metadata: Optional metadata to associate with the memory. + agent: Optional agent identifier. + + Raises: + ValueError: If the item's timestamp is in the future. + """ + import logging + item = ShortTermMemoryItem(data=value, metadata=metadata, agent=agent) + + if item.timestamp > datetime.now(): + raise ValueError("Cannot save memory item with future timestamp") + + logging.debug(f"Saving memory item with timestamp: {item.timestamp}") + if self._memory_provider == "mem0": item.data = f"Remember the following insights from Agent run: {item.data}" diff --git a/src/crewai/memory/short_term/short_term_memory_item.py b/src/crewai/memory/short_term/short_term_memory_item.py index 68c66be10..ea3197d00 100644 --- a/src/crewai/memory/short_term/short_term_memory_item.py +++ b/src/crewai/memory/short_term/short_term_memory_item.py @@ -10,6 +10,8 @@ class ShortTermMemoryItem: metadata: Optional[Dict[str, Any]] = None, timestamp: Optional[datetime] = None, ): + if timestamp is not None and timestamp > datetime.now(): + raise ValueError("Timestamp cannot be in the future") self.data = data self.agent = agent self.metadata = metadata if metadata is not None else {} diff --git a/src/crewai/memory/storage/rag_storage.py b/src/crewai/memory/storage/rag_storage.py index 5b9b0c491..05c7c073a 100644 --- a/src/crewai/memory/storage/rag_storage.py +++ b/src/crewai/memory/storage/rag_storage.py @@ -115,7 +115,25 @@ class RAGStorage(BaseRAGStorage): filter: Optional[dict] = None, score_threshold: float = 0.35, recency_weight: float = 0.3, + time_decay_days: float = 1.0, ) -> List[Any]: + """ + Search for entries in the storage based on semantic similarity and recency. + + Args: + query: The search query string. + limit: Maximum number of results to return. + filter: Optional filter to apply to the search. + score_threshold: Minimum score threshold for results. + recency_weight: Weight given to recency vs. semantic similarity (0.0-1.0). + Higher values prioritize recent memories more strongly. + time_decay_days: Number of days over which recency factor decays to zero. + Smaller values make older memories lose relevance faster. + + Returns: + List of search results, each containing id, metadata, context, and score. + Results are sorted by combined semantic similarity and recency score. + """ if not hasattr(self, "app"): self._initialize_app() @@ -140,7 +158,7 @@ class RAGStorage(BaseRAGStorage): now = datetime.now() # Calculate recency factor (newer = higher score) time_diff_seconds = (now - timestamp).total_seconds() - recency_factor = max(0, 1 - (time_diff_seconds / (24 * 60 * 60))) # Normalize to 1 day + recency_factor = max(0, 1 - (time_diff_seconds / (time_decay_days * 24 * 60 * 60))) # Adjust score with recency factor result["score"] = result["score"] * (1 - recency_weight) + recency_factor * recency_weight except (ValueError, TypeError): diff --git a/tests/memory/test_memory_topic_changes.py b/tests/memory/test_memory_topic_changes.py index c264d2607..a82341575 100644 --- a/tests/memory/test_memory_topic_changes.py +++ b/tests/memory/test_memory_topic_changes.py @@ -85,3 +85,46 @@ def test_memory_prioritizes_recent_topic(short_term_memory): # Verify that the scores reflect the recency prioritization assert results[0]["score"] > results[1]["score"], "Recent topic should have higher score" + + +def test_future_timestamp_validation(): + """Test that ShortTermMemoryItem raises ValueError for future timestamps.""" + # Setup agent and task for memory + agent = Agent( + role="Tutor", + goal="Teach programming concepts", + backstory="You are a programming tutor helping students learn.", + tools=[], + verbose=True, + ) + + task = Task( + description="Explain programming concepts to students.", + expected_output="Clear explanations of programming concepts.", + agent=agent, + ) + + # Create a future timestamp + future_timestamp = datetime.now() + timedelta(days=1) + + # Test constructor validation + with pytest.raises(ValueError, match="Timestamp cannot be in the future"): + ShortTermMemoryItem(data="Test data", timestamp=future_timestamp) + + # Test save method validation + memory = ShortTermMemory(crew=Crew(agents=[agent], tasks=[task])) + + # Create a memory item with a future timestamp + future_data = "Test data with future timestamp" + + # We need to pass the data directly to the save method + # The save method will create a ShortTermMemoryItem internally + # and then we'll modify its timestamp before it's saved + + # Mock datetime.now to return a fixed time + with patch('crewai.memory.short_term.short_term_memory_item.datetime') as mock_datetime: + # Set up the mock to return our future timestamp when now() is called + mock_datetime.now.return_value = future_timestamp + + with pytest.raises(ValueError, match="Cannot save memory item with future timestamp"): + memory.save(value=future_data)