-
Notifications
You must be signed in to change notification settings - Fork 0
Extract conversation history into thread-safe class with automatic trimming #21
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
Merged
hiyouga
merged 7 commits into
main
from
copilot/refactor-conversation-history-management
Jan 1, 2026
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d8c7df3
Initial plan
Copilot 9eb7ea8
Refactor conversation history management into thread-safe class
Copilot 99e896d
Remove redundant check in ConversationHistory.get_recent_messages
Copilot a19b93e
Add automatic history trimming to prevent memory leaks
Copilot ced71f5
Simplify ConversationHistory: merge into PrettyGeminiBot, remove para…
Copilot 892d337
Add make test command and update documentation
Copilot 950d51e
Use pytest for tests, add pytest dependency, and create test workflow
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| name: Tests | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| pull_request: | ||
| branches: | ||
| - main | ||
|
|
||
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v5 | ||
| with: | ||
| python-version: '3.11' | ||
| github-token: ${{ github.token }} | ||
|
|
||
| - name: Run tests | ||
| run: make test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| """Unit tests for ConversationHistory class.""" | ||
|
|
||
| import os | ||
| import threading | ||
| import time | ||
|
|
||
| from mini_ema.bot.pretty_gemini_bot import ConversationHistory | ||
|
|
||
|
|
||
| def test_initialization(): | ||
| """Test basic initialization of ConversationHistory.""" | ||
| # Set env var for testing | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "5" | ||
| history = ConversationHistory() | ||
| assert history._max_capacity == 10 # 5 rounds * 2 messages per round | ||
| assert len(history._history) == 0 | ||
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| assert history.get_recent_messages() == [] | ||
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def test_add_messages(): | ||
| """Test adding messages to history.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "5" | ||
| history = ConversationHistory() | ||
| messages = ["user message", "assistant response"] | ||
| history.add_messages(messages) | ||
| assert len(history._history) == 2 | ||
| assert history.get_recent_messages() == messages | ||
|
|
||
|
|
||
| def test_get_recent_messages_basic(): | ||
| """Test getting recent messages with basic scenarios.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "3" | ||
| history = ConversationHistory() | ||
|
|
||
| # Add 2 rounds (4 messages) | ||
| history.add_messages(["user1", "assistant1"]) | ||
| history.add_messages(["user2", "assistant2"]) | ||
|
|
||
| # Get all messages | ||
| recent = history.get_recent_messages() | ||
| assert recent == ["user1", "assistant1", "user2", "assistant2"] | ||
|
|
||
|
|
||
| def test_automatic_trimming(): | ||
| """Test that history automatically trims to max_capacity.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "2" | ||
| history = ConversationHistory() # max_capacity = 4 | ||
|
|
||
| # Add 4 rounds (8 messages) | ||
| history.add_messages(["user1", "assistant1"]) | ||
| history.add_messages(["user2", "assistant2"]) | ||
| history.add_messages(["user3", "assistant3"]) | ||
| history.add_messages(["user4", "assistant4"]) | ||
|
|
||
| # Should only keep last 2 rounds (4 messages) | ||
| recent = history.get_recent_messages() | ||
| assert recent == ["user3", "assistant3", "user4", "assistant4"] | ||
| assert len(recent) == 4 | ||
|
|
||
|
|
||
| def test_automatic_trimming_on_add(): | ||
| """Test that history is automatically trimmed when adding messages.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "2" | ||
| history = ConversationHistory() # max_capacity = 4 | ||
|
|
||
| # Add 1 round | ||
| history.add_messages(["user1", "assistant1"]) | ||
| assert len(history._history) == 2 | ||
|
|
||
| # Add 1 more round | ||
| history.add_messages(["user2", "assistant2"]) | ||
| assert len(history._history) == 4 | ||
|
|
||
| # Add 1 more round - should trim the oldest round | ||
| history.add_messages(["user3", "assistant3"]) | ||
| assert len(history._history) == 4 # Should still be 4 (not 6) | ||
| assert history.get_recent_messages() == ["user2", "assistant2", "user3", "assistant3"] | ||
|
|
||
| # Add another round - should trim again | ||
| history.add_messages(["user4", "assistant4"]) | ||
| assert len(history._history) == 4 | ||
| assert history.get_recent_messages() == ["user3", "assistant3", "user4", "assistant4"] | ||
|
|
||
|
|
||
| def test_clear(): | ||
| """Test clearing conversation history.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "5" | ||
| history = ConversationHistory() | ||
|
|
||
| # Add messages | ||
| history.add_messages(["user1", "assistant1"]) | ||
| assert len(history._history) == 2 | ||
|
|
||
| # Clear history | ||
| history.clear() | ||
| assert len(history._history) == 0 | ||
| assert history.get_recent_messages() == [] | ||
|
|
||
|
|
||
| def test_empty_history(): | ||
| """Test operations on empty history.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "5" | ||
| history = ConversationHistory() | ||
|
|
||
| assert len(history._history) == 0 | ||
| assert history.get_recent_messages() == [] | ||
|
|
||
| # Clear empty history should not raise error | ||
| history.clear() | ||
| assert len(history._history) == 0 | ||
|
|
||
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def test_zero_max_rounds(): | ||
| """Test when max_rounds is 0.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "0" | ||
| history = ConversationHistory() | ||
|
|
||
| # max_capacity should be 0 | ||
| assert history._max_capacity == 0 | ||
|
|
||
| # Add messages | ||
| history.add_messages(["user1", "assistant1"]) | ||
|
|
||
| # Should return empty list | ||
| recent = history.get_recent_messages() | ||
| assert recent == [] | ||
| assert len(history._history) == 0 | ||
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def test_thread_safety(): | ||
| """Test thread safety of ConversationHistory operations.""" | ||
| os.environ["PRETTY_GEMINI_BOT_HISTORY_LENGTH"] = "100" | ||
| history = ConversationHistory() | ||
| errors = [] | ||
|
|
||
| def add_messages_thread(thread_id, count): | ||
| """Add messages from a thread.""" | ||
| try: | ||
| for i in range(count): | ||
| history.add_messages([f"user_{thread_id}_{i}", f"assistant_{thread_id}_{i}"]) | ||
| except Exception as e: | ||
| errors.append(e) | ||
|
|
||
| def read_messages_thread(count): | ||
| """Read messages from a thread.""" | ||
| try: | ||
| for _ in range(count): | ||
| history.get_recent_messages() | ||
| time.sleep(0.001) # Small delay to interleave operations | ||
| except Exception as e: | ||
| errors.append(e) | ||
|
|
||
| # Create multiple threads that add and read messages concurrently | ||
| threads = [] | ||
| for i in range(5): | ||
| t = threading.Thread(target=add_messages_thread, args=(i, 10)) | ||
| threads.append(t) | ||
| t.start() | ||
|
|
||
| for _ in range(3): | ||
| t = threading.Thread(target=read_messages_thread, args=(20,)) | ||
| threads.append(t) | ||
| t.start() | ||
|
|
||
| # Wait for all threads to complete | ||
| for t in threads: | ||
| t.join() | ||
|
|
||
| # Check no errors occurred | ||
| assert len(errors) == 0 | ||
|
|
||
| # Verify we don't have more than max_capacity messages | ||
| assert len(history._history) <= 200 # 100 rounds * 2 messages | ||
hiyouga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| # Run all tests | ||
| test_initialization() | ||
| test_add_messages() | ||
| test_get_recent_messages_basic() | ||
| test_automatic_trimming() | ||
| test_automatic_trimming_on_add() | ||
| test_clear() | ||
| test_empty_history() | ||
| test_zero_max_rounds() | ||
| test_thread_safety() | ||
| print("All tests passed!") | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.