feat: Implement memory sharing for EvaluatorOptimizerLLM #227
Conversation
- Fix message parameter name typo - Ensure proper request_params handling in next step generation - Maintain consistency with other method signatures
- Add init_queue method to prevent redundant queue initialization. - Ensure proper queue initialization before starting the event bus to enhance asynchronous task execution efficiency. Previously, AsyncEventBus was instantiated during logger initialization, which occurs upon import. This led to uncontrollable queue binding. By initializing the queue during the `start` method, reliable queue binding is ensured in a multi-threaded environment.
- Add a shared memory parameter to the EvaluatorOptimizerLLM class. - Implement the `share_memory_from` method to enable memory sharing functionality. This change aims to optimize memory management between the evaluator and optimizer. For example, the `optimizer_llm` is often bound to an MCP server. After invoking the MCP and receiving content, the LLM might sometimes "hallucinate" or produce responses inconsistent with the MCP's returned content. The `evaluator_llm`, when performing its assessment, needs access to the `optimizer_llm`'s memory to better determine if it is hallucinating or generating unexpected output. MeanWhile, optimizer_llm can do better work when having evaluator_llm's memory.
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EvaluatorOptimizerLLM
participant OptimizerLLM
participant EvaluatorLLM
User->>EvaluatorOptimizerLLM: Instantiate (share_memory=True)
EvaluatorOptimizerLLM->>OptimizerLLM: Initialize
EvaluatorOptimizerLLM->>EvaluatorLLM: Initialize
EvaluatorOptimizerLLM->>EvaluatorLLM: share_memory_from(OptimizerLLM)
EvaluatorLLM->>OptimizerLLM: Access history
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
src/mcp_agent/workflows/evaluator_optimizer/evaluator_optimizer.py (1)
152-153: Memory sharing implementation is correct for the stated use case.The implementation correctly shares memory from the optimizer to the evaluator after both LLMs are initialized, which aligns with the PR objective of allowing the evaluator to assess optimizer hallucinations.
However, consider whether bidirectional memory sharing would be beneficial, as mentioned in the PR objectives that "the optimizer LLM can also benefit from having access to the evaluator LLM's memory."
If bidirectional sharing is desired, consider adding:
if share_memory: self.evaluator_llm.share_memory_from(self.optimizer_llm) + # Optionally enable bidirectional sharing + # self.optimizer_llm.share_memory_from(self.evaluator_llm)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp_agent/workflows/evaluator_optimizer/evaluator_optimizer.py(3 hunks)src/mcp_agent/workflows/llm/augmented_llm.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/mcp_agent/workflows/evaluator_optimizer/evaluator_optimizer.py (1)
src/mcp_agent/workflows/llm/augmented_llm.py (1)
share_memory_from(277-278)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: checks / test
🔇 Additional comments (1)
src/mcp_agent/workflows/evaluator_optimizer/evaluator_optimizer.py (1)
78-78: Parameter addition looks good.The
share_memoryparameter is appropriately added with a sensible default value ofFalse.
There was a problem hiding this comment.
Sorry for the delay @jingx8885, and thanks for the contribution. In general supportive of this change, but I have questions of whether this works well in practice. Also would appreciate adding a test for this scenario as part of your change.
| @@ -274,6 +274,26 @@ def __init__( | |||
| self.model_selector = self.context.model_selector | |||
| self.type_converter = type_converter | |||
|
|
|||
| def share_memory_from(self, other: "AugmentedLLM"): | |||
There was a problem hiding this comment.
I would prefer not to add this to the main API, since someone can just do
llm.history = other_llm.history
themselves
| if share_memory: | ||
| self.evaluator_llm.share_memory_from(self.optimizer_llm) |
There was a problem hiding this comment.
This is very interesting @jingx8885, but I'm curious if this works well in practice.
I understand your point that the evaluator should consider the full interaction to be able to determine the quality. But does it catch the issues you mentioned in the PR description when it has access to the full history? Also, does the generator get confused in follow-up interactions, or does it work well?
feat: Implement memory sharing for EvaluatorOptimizerLLM
share_memory_frommethod to enable memory sharing functionality.This change aims to optimize memory management between the evaluator and optimizer.
For example, the
optimizer_llmis often bound to an MCP server. After invoking the MCP and receiving content, the LLM might sometimes "hallucinate" or produce responses inconsistent with the MCP's returned content. Theevaluator_llm, when performing its assessment, needs access to theoptimizer_llm's memory to better determine if it is hallucinating or generating unexpected output. MeanWhile, optimizer_llm can do better work when having evaluator_llm's memory.Summary by CodeRabbit