-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add per_turn parameter to SlidingWindowConversationManager #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Enable SlidingWindowConversationManager to apply message management during the agent loop execution by implementing per_turn functionality. Changes: - Add per_turn parameter (bool | int, default: False) to control when message management is applied - Implement register_hooks method to register AfterToolCallEvent callback - Add _on_after_tool_call callback to apply management based on per_turn - Update get_state and restore_from_session to persist _tool_call_count - Auto-register conversation_manager as hook in Agent.__init__ if it implements register_hooks method - Add comprehensive test suite with 25 test cases Resolves #3
| Args: | ||
| event: The after tool call event containing the agent and tool execution details. | ||
| """ | ||
| self._tool_call_count += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Track model calls instead of tool calls, since you could have N tools execute per "turn"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Will change to track model calls using AfterModelCallEvent instead.
src/strands/agent/agent.py
Outdated
| self.hooks.add_hook(self._session_manager) | ||
|
|
||
| # Check if conversation_manager can provide hooks | ||
| if hasattr(self.conversation_manager, "register_hooks"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, check if it derives from HookProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update to use isinstance check with HookProvider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tests for the conversation manager (search for test_sliding_window_conversation_manager_with_tool_results_truncated) move these to be in ther
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will move tests to existing test_conversation_manager.py file.
| Optional list of messages to prepend to the agent's messages. | ||
| """ | ||
| result = super().restore_from_session(state) | ||
| self._tool_call_count = state.get("_tool_call_count", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to use an underscore since we own the implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove underscore prefix from model_call_count.
|
/strands |
Changes based on PR review feedback: - Track model calls instead of tool calls using AfterModelCallEvent - Rename counter from _tool_call_count to model_call_count (remove underscore) - Use hasattr/callable check instead of isinstance for HookProvider protocol - Move tests from standalone file to existing test_conversation_manager.py All 40 tests passing (15 existing + 25 new).
src/strands/agent/conversation_manager/sliding_window_conversation_manager.py
Outdated
Show resolved
Hide resolved
| - True: Apply management after every model call | ||
| - int (e.g., 3): Apply management after every N model calls | ||
| When to use per_turn: If your agent performs many tool operations in loops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this much more concise. Just talk about how this can reduce the context that the agent receives which can speed up agents and/or reduce costs, at the expense of removing context that the agent uses to guide the next step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will simplify to focus on context reduction benefits vs tradeoffs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't make this more concise
src/strands/agent/conversation_manager/sliding_window_conversation_manager.py
Outdated
Show resolved
Hide resolved
|
/strands |
Changes: - Mark HookProvider as @runtime_checkable and use isinstance check in Agent - Change from AfterModelCallEvent to BeforeModelCallEvent for better timing - Move per_turn check to callback for dynamic changes - Rename model_call_count (removed underscore) - Simplify per_turn docstring to focus on context reduction tradeoffs - Convert test classes to concise function-style tests matching existing patterns - Elevate all imports to top of file - Remove implementation detail tests and focus on behavior - Add test for dynamic per_turn changes - Remove test_per_turn_updated.log file All 24 tests passing (15 existing + 9 new concise per_turn tests)
|
/strands |
| assert mock.call_count >= 1 | ||
|
|
||
|
|
||
| def test_per_turn_integer_manages_every_n_calls(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up above, we have a bunch of tests that use data-driven test cases.
Many of these tests overlap with that - Rewrite the tests that make sense as additional test-cases
|
/strands |
Changes: - Make per_turn docstring much more concise (2 sentences vs 5) - Use BeforeModelCallEvent instead of AfterModelCallEvent for better timing - Move per_turn check to callback (always register, check in callback for dynamic changes) - Mark HookProvider as @runtime_checkable and use isinstance in Agent - Convert tests to data-driven parametrized style matching existing patterns - Consolidate 9 function tests into 2 parametrized tests + 2 focused tests - Remove implementation detail tests (hook registration checks) - Elevate imports to top of file (remove local imports) - Remove test log file All 27 tests passing (15 existing + 12 new per_turn tests)
|
/strands update the PR description according to the guidelines |
Problem
When agents perform many tool operations (e.g., web browsing with screenshots), message history grows rapidly and the agent loop slows down. Currently,
ConversationManager.apply_management()only runs at the end of execution in the finally block, causing performance degradation during long-running agent loops.Solution
Add
per_turnparameter toSlidingWindowConversationManagerthat enables proactive message management during the agent loop by leveraging the hooks system.Resolves #3
Public API Changes
SlidingWindowConversationManager
Before:
After:
Parameter Options
per_turn=False(default): Current behavior - management only at endper_turn=True: Apply management before every model callper_turn=N(int): Apply management before every N model callsHookProvider Protocol
Added
@runtime_checkabletoHookProviderprotocol to enable proper isinstance checks.Implementation Approach
SlidingWindowConversationManagerimplementsHookProvider.register_hooks()and subscribes toBeforeModelCallEventper_turndynamically (not at registration) to support runtime changesmodel_call_countfor session restorationapply_management()for robustnessImpact
Benefits:
Tradeoffs:
per_turnvalueBackward Compatibility
✓ Default
per_turn=Falsemaintains current behavior✓ No breaking changes
✓ Other conversation managers (Summarizing, Null) unaffected