fix: accumulate additive events in memory staging area#165
fix: accumulate additive events in memory staging area#165
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 91.20% 91.48% +0.27%
==========================================
Files 19 19
Lines 1592 1632 +40
==========================================
+ Hits 1452 1493 +41
+ Misses 140 139 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR implements additive semantics for certain event types in the memory staging area to fix data loss when multiple events of the same type occur within a single simulation step. Messages and actions now accumulate in lists rather than overwriting each other. Supporting changes update memory rendering and communication history retrieval to handle list-valued content. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
mesa_llm/memory/memory.py (2)
56-57: Minor formatting inconsistency for non-dict list items.Line 57 appends a trailing colon after non-dict items (
{item} :), but since these are values rather than key-value pairs, the colon appears unnecessary and inconsistent with how scalar values are displayed elsewhere (line 61 also has this pattern for non-list scalars).💡 Suggested fix
else: - lines.append(f" [blue]└──[/blue] [cyan]{item} :[/cyan]") + lines.append(f" [blue]└──[/blue] [cyan]{item}[/cyan]")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mesa_llm/memory/memory.py` around lines 56 - 57, The list branch currently appends non-dict items with a trailing colon using the lines.append call that builds f" [blue]└──[/blue] [cyan]{item} :[/cyan]"; remove the colon so scalar list items render like other scalars (match the pattern used elsewhere for non-list scalars, e.g., the similar append at the non-list scalar branch), updating the string to omit " :" and ensure whitespace/formatting matches other scalar lines.
143-146: Type annotation mismatch:frozensetassigned toset[str].The annotation declares
set[str]but the value is afrozenset. While this works at runtime, it's technically incorrect for static type checkers and could mislead subclasses about mutability.💡 Suggested fix
- ADDITIVE_EVENT_TYPES: set[str] = frozenset({"message", "action"}) + ADDITIVE_EVENT_TYPES: frozenset[str] = frozenset({"message", "action"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mesa_llm/memory/memory.py` around lines 143 - 146, The annotation for ADDITIVE_EVENT_TYPES is incorrect: it declares set[str] but is assigned a frozenset; update the type to match immutability by changing the annotation to frozenset[str] (or typing.FrozenSet[str]) so ADDITIVE_EVENT_TYPES: frozenset[str] = frozenset({"message", "action"}) and keep the existing value; this ensures static type checkers and readers see the correct immutable type for the constant.mesa_llm/memory/st_memory.py (1)
97-111: LGTM with a consistency note.The implementation correctly handles both list-valued and scalar message entries. However,
EpisodicMemory.get_communication_history()(lines 150-160) directly accessesentry.content['message']without checking if it's a list, unlike the updated approach here. If both classes are used with additive message storage, they should handle messages consistently to avoid formatting errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mesa_llm/memory/st_memory.py` around lines 97 - 111, EpisodicMemory.get_communication_history currently assumes entry.content['message'] is a scalar; change it to match the logic used in get_communication_history of the short-term memory: first check that "message" exists in entry.content, retrieve msgs = entry.content["message"], then if isinstance(msgs, list) iterate and append each msg with the same formatting (e.g., f"step {entry.step}: {msg}\n\n") else append the scalar message with the same formatting; ensure you reference EpisodicMemory.get_communication_history and use entry.step and entry.content consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mesa_llm/memory/memory.py`:
- Around line 56-57: The list branch currently appends non-dict items with a
trailing colon using the lines.append call that builds f" [blue]└──[/blue]
[cyan]{item} :[/cyan]"; remove the colon so scalar list items render like other
scalars (match the pattern used elsewhere for non-list scalars, e.g., the
similar append at the non-list scalar branch), updating the string to omit " :"
and ensure whitespace/formatting matches other scalar lines.
- Around line 143-146: The annotation for ADDITIVE_EVENT_TYPES is incorrect: it
declares set[str] but is assigned a frozenset; update the type to match
immutability by changing the annotation to frozenset[str] (or
typing.FrozenSet[str]) so ADDITIVE_EVENT_TYPES: frozenset[str] =
frozenset({"message", "action"}) and keep the existing value; this ensures
static type checkers and readers see the correct immutable type for the
constant.
In `@mesa_llm/memory/st_memory.py`:
- Around line 97-111: EpisodicMemory.get_communication_history currently assumes
entry.content['message'] is a scalar; change it to match the logic used in
get_communication_history of the short-term memory: first check that "message"
exists in entry.content, retrieve msgs = entry.content["message"], then if
isinstance(msgs, list) iterate and append each msg with the same formatting
(e.g., f"step {entry.step}: {msg}\n\n") else append the scalar message with the
same formatting; ensure you reference EpisodicMemory.get_communication_history
and use entry.step and entry.content consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3cee3f10-7388-4e91-98bc-3ecaacc8aac4
📒 Files selected for processing (5)
mesa_llm/memory/memory.pymesa_llm/memory/st_lt_memory.pymesa_llm/memory/st_memory.pytests/test_llm_agent.pytests/test_memory/test_memory_staging.py
There was a problem hiding this comment.
Thank you! This would probably have been the approach that I'd have gone with as well. I haven't done a thorough check of the code, but the idea seems right to me. I will wait for @colinfrisch’s approval as well before merging:)
yashhzd
force-pushed
the
fix/memory-staging-overwrite-137
branch
from
3ff130d to
bcd7d35
Compare
yashhzd
force-pushed
the
fix/memory-staging-overwrite-137
branch
from
bcd7d35 to
a5a4b2d
Compare
The base Memory.add_to_memory() method uses a plain dict where each event type maps to a single value. When multiple events of the same type occur within a single step, subsequent calls silently overwrite previous entries, causing permanent data loss. Introduces ADDITIVE_EVENT_TYPES (message, action) that accumulate in lists rather than overwriting. Updates MemoryEntry.__str__, get_communication_history(), and adds 9 focused tests. Closes mesa#137
yashhzd
force-pushed
the
fix/memory-staging-overwrite-137
branch
from
a5a4b2d to
675d638
Compare
Fixes ruff PLC0415 (import should be at top-level) by moving the EpisodicMemory import from inside test functions to the top of the file.
|
Hi @sanika-n and @yashhzd, I have updated this PR by making additive event types a configurable parameter in Memory classes (except memory = ShortTermMemory(
agent=agent,
additive_event_types=["message", "observation"],
)I have also updated docstrings and tests, and merged the latest |
4 participants
Summary
Fixes #137
The base
Memory.add_to_memory()method uses a plain dict (step_content) where each event type maps to a single value. When multiple events of the same type occur within a single step (e.g., an agent receiving messages from several neighbors), subsequent calls silently overwrite previous entries, causing permanent data loss.Changes
memory.py: IntroduceADDITIVE_EVENT_TYPES(frozenset({"message", "action"})) — events of these types are collected in a list rather than overwritten. Observations remain state-based (overwrite semantics preserved). Other types (plans, reasoning steps, etc.) also keep overwrite semantics.memory.py(MemoryEntry.__str__): Updated to render list-valued content with numbered sub-entries for clean display.st_memory.py/st_lt_memory.py: Updatedget_communication_history()to iterate over list-valued message entries.test_llm_agent.py: Updatedtest_apply_plan_adds_to_memoryassertion to match the new list-based action storage.tests/test_memory/test_memory_staging.py— 9 focused tests covering:MemoryEntry.__str__formats lists properlyShortTermMemory.get_communication_historyhandles list messagesDesign Decision
Per the discussion in #76, communicative events (messages, actions) should be additive — an agent receiving five messages in one step should have all five recorded. Observations may remain state-based to prevent LLM context bloat.
The
ADDITIVE_EVENT_TYPESset is a class attribute onMemory, making it easy for subclasses to extend with additional event types if needed.Test Plan
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes