-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix cache resource leaks and test failures #8744
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
- Add proper cleanup methods to Cache class (close, __del__) - Add context manager support (__enter__, __exit__) for automatic resource management - Update test fixtures to properly cleanup cache instances - Fix SQLite database and file descriptor exhaustion issues during test runs These changes resolve intermittent test failures caused by resource leaks in the FanoutCache implementation, ensuring tests run reliably both individually and as part of the full test suite.
Thanks @armoucar !! This surprises me. Does this PR actually change/fix issues? |
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.
Pull Request Overview
This PR fixes resource leaks in the Cache class that were causing SQLite database and file descriptor errors during testing. The changes add proper resource cleanup mechanisms to prevent file descriptor exhaustion from unclosed database connections.
- Added cleanup methods (
close()
,__del__()
) and context manager support to the Cache class - Updated test fixtures to use proper resource cleanup with
yield
and explicit cleanup calls - Modified individual tests to explicitly clean up cache instances
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
dspy/clients/cache.py | Added resource cleanup methods and context manager support to Cache class |
tests/clients/test_cache.py | Updated cache fixture to use yield pattern and modified tests to use context managers |
tests/clients/test_lm.py | Added explicit cache cleanup calls and minor formatting changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
except Exception as e: | ||
logger.debug(f"Failed to close disk cache: {e}") | ||
|
||
if self.enable_memory_cache and hasattr(self.memory_cache, "clear"): |
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.
The condition hasattr(self.memory_cache, "clear")
will always be false when self.memory_cache
is an empty dict ({}
). When memory cache is disabled, self.memory_cache = {}
in the constructor, and dictionaries always have a clear()
method. This means the clear operation will be attempted even when memory cache is disabled, which is inconsistent with the intended behavior.
if self.enable_memory_cache and hasattr(self.memory_cache, "clear"): | |
if self.enable_memory_cache: |
Copilot uses AI. Check for mistakes.
"content": [ | ||
{"type": "output_text", "text": expected_text, "annotations": []} | ||
], | ||
"content": [{"type": "output_text", "text": expected_text, "annotations": []}], |
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.
[nitpick] This formatting change appears to be unrelated to the cache resource leak fix. The original multi-line format was more readable for this nested dictionary structure.
"content": [{"type": "output_text", "text": expected_text, "annotations": []}], | |
"content": [ | |
{ | |
"type": "output_text", | |
"text": expected_text, | |
"annotations": [], | |
} | |
], |
Copilot uses AI. Check for mistakes.
Problem
During local testing, I encountered intermittent test failures with SQLite database and file descriptor errors:
Root Cause
The issue was caused by resource leaks in the Cache class:
DSPY_CACHE
instance creates FanoutCache with 16 database shardsclose()
or__del__()
methods to properly close FanoutCache instancesSolution
close()
,__del__()
)__enter__()
,__exit__()
) for automatic resource managementyield
and explicit cleanupTesting
All previously failing tests now pass:
test_dspy_cache
test_async_lm_call_with_cache
test_save_and_load_memory_cache
test_request_cache_decorator
Full cache test suite (21 tests) passes without issues.
Files Changed
dspy/clients/cache.py
: Added resource cleanup and context manager supporttests/clients/test_cache.py
: Updated fixtures and tests for proper cleanuptests/clients/test_lm.py
: Added explicit cleanup for cache instances