mirror of
https://github.com/agentscope-ai/ReMe.git
synced 2026-08-28 05:25:04 +00:00
fix: surface summarize/retrieve failures instead of masking them (#160)
* fix(memory): surface summarize and retrieve failures clearly * fix(memory): make raise_exception configurable * fix(tests): resolve flake8 and pylint errors in error handling tests - Remove unnecessary sys.path.insert hack - Add module/class/function docstrings - Initialize call_kwargs in __init__ to fix W0201 - Suppress W0212 with inline pylint disable for _started access - Remove unnecessary lambda wrappers (W0108)
This commit is contained in:
Xinmin Zeng
GitHub
parent
9cb8dc834e
commit
bf79986f9c
2 changed files with 188 additions and 10 deletions
66
reme/reme.py
66
reme/reme.py
|
|
@ -162,6 +162,28 @@ class ReMe(Application):
|
|||
|
||||
return memory_type, memory_target
|
||||
|
||||
def _ensure_started(self) -> None:
|
||||
"""Ensure memory operations run only after services are initialized."""
|
||||
if not self._started:
|
||||
raise RuntimeError("ReMe is not started. Call `await reme.start()` before using memory APIs.")
|
||||
|
||||
@staticmethod
|
||||
def _unwrap_memory_result(
|
||||
result: str | dict,
|
||||
operation_name: str,
|
||||
return_dict: bool,
|
||||
) -> str | dict:
|
||||
"""Normalize memory API results and fail loudly on swallowed inner errors."""
|
||||
if not isinstance(result, dict):
|
||||
raise RuntimeError(f"{operation_name} failed before producing a structured result: {result}")
|
||||
|
||||
if "answer" not in result:
|
||||
raise RuntimeError(f"{operation_name} returned an invalid result payload: missing 'answer'")
|
||||
|
||||
if return_dict:
|
||||
return result
|
||||
return result["answer"]
|
||||
|
||||
async def summarize_memory(
|
||||
self,
|
||||
messages: list[Message | dict],
|
||||
|
|
@ -173,10 +195,12 @@ class ReMe(Application):
|
|||
version: str = "default",
|
||||
retrieve_top_k: int = 20,
|
||||
return_dict: bool = False,
|
||||
raise_exception: bool = False,
|
||||
llm_config_name: str = "default",
|
||||
**kwargs,
|
||||
) -> str | dict:
|
||||
"""Summarize personal, procedural and tool memories for the given context."""
|
||||
self._ensure_started()
|
||||
format_messages: list[Message] = []
|
||||
for message in messages:
|
||||
if isinstance(message, dict):
|
||||
|
|
@ -192,12 +216,14 @@ class ReMe(Application):
|
|||
enable_when_to_use=False,
|
||||
enable_multiple=True,
|
||||
top_k=retrieve_top_k,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
AddMemory(
|
||||
enable_thinking_params=enable_thinking_params,
|
||||
enable_memory_target=False,
|
||||
enable_when_to_use=False,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
]
|
||||
if self.enable_profile:
|
||||
|
|
@ -207,18 +233,21 @@ class ReMe(Application):
|
|||
enable_thinking_params=False,
|
||||
enable_memory_target=False,
|
||||
profile_dir=self.profile_dir,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
UpdateProfilesV1(
|
||||
enable_thinking_params=enable_thinking_params,
|
||||
enable_memory_target=False,
|
||||
enable_multiple=True,
|
||||
profile_dir=self.profile_dir,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
],
|
||||
)
|
||||
personal_summarizer: BaseMemoryAgent = PersonalSummarizer(
|
||||
llm=llm_config_name,
|
||||
tools=personal_summarizer_tools,
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
|
||||
else:
|
||||
|
|
@ -233,14 +262,17 @@ class ReMe(Application):
|
|||
enable_when_to_use=False,
|
||||
enable_multiple=True,
|
||||
top_k=retrieve_top_k,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
AddMemory(
|
||||
enable_thinking_params=enable_thinking_params,
|
||||
enable_memory_target=False,
|
||||
enable_when_to_use=False,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
],
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
tool_summarizer: BaseMemoryAgent = ToolSummarizer(
|
||||
llm=llm_config_name,
|
||||
|
|
@ -251,14 +283,17 @@ class ReMe(Application):
|
|||
enable_when_to_use=False,
|
||||
enable_multiple=True,
|
||||
top_k=retrieve_top_k,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
AddMemory(
|
||||
enable_thinking_params=enable_thinking_params,
|
||||
enable_memory_target=False,
|
||||
enable_when_to_use=False,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
],
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
|
||||
memory_agents = []
|
||||
|
|
@ -306,7 +341,11 @@ class ReMe(Application):
|
|||
memory_agents = [personal_summarizer, procedural_summarizer, tool_summarizer]
|
||||
|
||||
reme_summarizer: BaseMemoryAgent = ReMeSummarizer(
|
||||
tools=[AddHistory(), DelegateTask(memory_agents=memory_agents)],
|
||||
tools=[
|
||||
AddHistory(raise_exception=raise_exception),
|
||||
DelegateTask(memory_agents=memory_agents, raise_exception=raise_exception),
|
||||
],
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
|
||||
result = await reme_summarizer.call(
|
||||
|
|
@ -317,10 +356,7 @@ class ReMe(Application):
|
|||
**kwargs,
|
||||
)
|
||||
|
||||
if return_dict:
|
||||
return result
|
||||
else:
|
||||
return result["answer"]
|
||||
return self._unwrap_memory_result(result, "summarize_memory", return_dict)
|
||||
|
||||
async def retrieve_memory(
|
||||
self,
|
||||
|
|
@ -335,10 +371,12 @@ class ReMe(Application):
|
|||
retrieve_top_k: int = 20,
|
||||
enable_time_filter: bool = True,
|
||||
return_dict: bool = False,
|
||||
raise_exception: bool = False,
|
||||
llm_config_name: str = "default",
|
||||
**kwargs,
|
||||
) -> str | dict:
|
||||
"""Retrieve relevant personal, procedural and tool memories for a query."""
|
||||
self._ensure_started()
|
||||
|
||||
if version == "default":
|
||||
personal_retriever_tools = []
|
||||
|
|
@ -348,6 +386,7 @@ class ReMe(Application):
|
|||
enable_thinking_params=False,
|
||||
enable_memory_target=False,
|
||||
profile_dir=self.profile_dir,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
)
|
||||
personal_retriever_tools.extend(
|
||||
|
|
@ -357,16 +396,19 @@ class ReMe(Application):
|
|||
enable_thinking_params=enable_thinking_params,
|
||||
enable_time_filter=enable_time_filter,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
ReadHistory(
|
||||
enable_thinking_params=enable_thinking_params,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
],
|
||||
)
|
||||
personal_retriever: BaseMemoryAgent = PersonalRetriever(
|
||||
llm=llm_config_name,
|
||||
tools=personal_retriever_tools,
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
else:
|
||||
raise NotImplementedError(f"version={version} is not supported")
|
||||
|
|
@ -379,12 +421,15 @@ class ReMe(Application):
|
|||
enable_thinking_params=enable_thinking_params,
|
||||
enable_time_filter=False,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
ReadHistory(
|
||||
enable_thinking_params=enable_thinking_params,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
],
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
tool_retriever: BaseMemoryAgent = ToolRetriever(
|
||||
llm=llm_config_name,
|
||||
|
|
@ -394,12 +439,15 @@ class ReMe(Application):
|
|||
enable_thinking_params=enable_thinking_params,
|
||||
enable_time_filter=False,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
ReadHistory(
|
||||
enable_thinking_params=enable_thinking_params,
|
||||
enable_multiple=True,
|
||||
raise_exception=raise_exception,
|
||||
),
|
||||
],
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
|
||||
memory_agents = []
|
||||
|
|
@ -444,7 +492,8 @@ class ReMe(Application):
|
|||
memory_agents = [personal_retriever, procedural_retriever, tool_retriever]
|
||||
|
||||
reme_retriever: BaseMemoryAgent = ReMeRetriever(
|
||||
tools=[DelegateTask(memory_agents=memory_agents)],
|
||||
tools=[DelegateTask(memory_agents=memory_agents, raise_exception=raise_exception)],
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
|
||||
result = await reme_retriever.call(
|
||||
|
|
@ -456,10 +505,7 @@ class ReMe(Application):
|
|||
**kwargs,
|
||||
)
|
||||
|
||||
if return_dict:
|
||||
return result
|
||||
else:
|
||||
return result["answer"]
|
||||
return self._unwrap_memory_result(result, "retrieve_memory", return_dict)
|
||||
|
||||
async def add_memory(
|
||||
self,
|
||||
|
|
|
|||
132
tests/test_reme_memory_error_handling.py
Normal file
132
tests/test_reme_memory_error_handling.py
Normal file
|
|
@ -0,0 +1,132 @@
|
|||
"""Tests for ReMe memory error handling and raise_exception propagation."""
|
||||
|
||||
import pytest
|
||||
|
||||
import reme.reme as reme_module
|
||||
from reme import ReMe
|
||||
|
||||
|
||||
class Recorder:
|
||||
"""Stub that records constructor args for later inspection."""
|
||||
|
||||
instances = []
|
||||
|
||||
def __init__(self, *args, **kwargs):
|
||||
self.args = args
|
||||
self.kwargs = kwargs
|
||||
self.call_kwargs = None
|
||||
Recorder.instances.append(self)
|
||||
|
||||
|
||||
class TopLevelAgent(Recorder):
|
||||
"""Stub agent that returns a successful structured result."""
|
||||
|
||||
async def call(self, **kwargs):
|
||||
"""Simulate a successful agent call."""
|
||||
self.call_kwargs = kwargs
|
||||
return {"answer": "ok", "success": True}
|
||||
|
||||
|
||||
def _make_reme() -> ReMe:
|
||||
"""Create a ReMe instance with startup bypassed for unit testing."""
|
||||
reme = ReMe(enable_logo=False, log_to_console=False, enable_profile=False)
|
||||
reme._started = True # pylint: disable=protected-access
|
||||
return reme
|
||||
|
||||
|
||||
def _patch_summarize_dependencies(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Patch all summarize-path dependencies with stubs."""
|
||||
Recorder.instances = []
|
||||
monkeypatch.setattr(reme_module, "AddDraftAndRetrieveSimilarMemory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "AddMemory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "AddHistory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "DelegateTask", Recorder)
|
||||
monkeypatch.setattr(reme_module, "PersonalSummarizer", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ProceduralSummarizer", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ToolSummarizer", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ReMeSummarizer", TopLevelAgent)
|
||||
|
||||
|
||||
def _patch_retrieve_dependencies(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Patch all retrieve-path dependencies with stubs."""
|
||||
Recorder.instances = []
|
||||
monkeypatch.setattr(reme_module, "RetrieveMemory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ReadHistory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "DelegateTask", Recorder)
|
||||
monkeypatch.setattr(reme_module, "PersonalRetriever", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ProceduralRetriever", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ToolRetriever", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ReMeRetriever", TopLevelAgent)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("raise_exception", [False, True])
|
||||
async def test_summarize_memory_propagates_raise_exception(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
raise_exception: bool,
|
||||
):
|
||||
"""Verify raise_exception is forwarded to every sub-agent in summarize."""
|
||||
_patch_summarize_dependencies(monkeypatch)
|
||||
reme = _make_reme()
|
||||
|
||||
result = await reme.summarize_memory(
|
||||
messages=[{"role": "user", "content": "hi", "time_created": "2026-03-20 10:00:00"}],
|
||||
task_name="demo-task",
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
|
||||
assert result == "ok"
|
||||
assert Recorder.instances
|
||||
assert all(instance.kwargs.get("raise_exception") is raise_exception for instance in Recorder.instances)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("raise_exception", [False, True])
|
||||
async def test_retrieve_memory_propagates_raise_exception(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
raise_exception: bool,
|
||||
):
|
||||
"""Verify raise_exception is forwarded to every sub-agent in retrieve."""
|
||||
_patch_retrieve_dependencies(monkeypatch)
|
||||
reme = _make_reme()
|
||||
|
||||
result = await reme.retrieve_memory(
|
||||
query="hello",
|
||||
task_name="demo-task",
|
||||
raise_exception=raise_exception,
|
||||
)
|
||||
|
||||
assert result == "ok"
|
||||
assert Recorder.instances
|
||||
assert all(instance.kwargs.get("raise_exception") is raise_exception for instance in Recorder.instances)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_summarize_memory_raises_runtime_error_for_unstructured_result(monkeypatch: pytest.MonkeyPatch):
|
||||
"""Verify RuntimeError is raised when the top-level summarizer returns a plain string."""
|
||||
Recorder.instances = []
|
||||
reme = _make_reme()
|
||||
|
||||
monkeypatch.setattr(reme_module, "PersonalSummarizer", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ProceduralSummarizer", Recorder)
|
||||
monkeypatch.setattr(reme_module, "ToolSummarizer", Recorder)
|
||||
monkeypatch.setattr(reme_module, "AddDraftAndRetrieveSimilarMemory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "AddMemory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "AddHistory", Recorder)
|
||||
monkeypatch.setattr(reme_module, "DelegateTask", Recorder)
|
||||
|
||||
class FailingTopLevelAgent(Recorder):
|
||||
"""Stub agent that returns a failure string instead of a dict."""
|
||||
|
||||
async def call(self, **kwargs):
|
||||
"""Simulate a failed agent call returning a plain error string."""
|
||||
self.call_kwargs = kwargs
|
||||
return "[ReMeSummarizer] failed: boom"
|
||||
|
||||
monkeypatch.setattr(reme_module, "ReMeSummarizer", FailingTopLevelAgent)
|
||||
|
||||
with pytest.raises(RuntimeError, match="summarize_memory failed before producing a structured result"):
|
||||
await reme.summarize_memory(
|
||||
messages=[{"role": "user", "content": "hi", "time_created": "2026-03-20 10:00:00"}],
|
||||
task_name="demo-task",
|
||||
)
|
||||
Loading…
Add table
Reference in a new issue