Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 18, 2025

The recent type annotation changes replaced dynamic lru_cache configuration with static @cache decorator on get_factual_value and get_target_value, breaking cache size configuration and causing memory leaks.

Issues identified

  • Ignored cache_size parameter: @cache is equivalent to lru_cache(maxsize=None). Tests explicitly using cache_size=0 to prevent memory exhaustion will fail.

  • Memory leak: @cache on instance methods holds references to self, preventing garbage collection. The original _setup_caching() avoided this by applying caching dynamically per-instance.

Context

The original implementation applied caching in __init__:

def _setup_caching(self):
    self.get_factual_value = lru_cache(self.cache_size)(self.get_factual_value)
    self.get_target_value = lru_cache(self.cache_size)(self.get_target_value)

This allowed per-instance cache configuration and proper garbage collection, as documented in the code comments referencing https://rednafi.com/python/lru_cache_on_methods/.

The new decorator-based approach:

@cache
def get_factual_value(self, engine: sa.engine.Engine) -> Any:
    ...

Cannot support configurable cache sizes or prevent holding references to constraint instances.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@codecov
Copy link

codecov bot commented Dec 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.58%. Comparing base (041ce7f) to head (4328769).
⚠️ Report is 2 commits behind head on ty.

Additional details and impacted files
@@           Coverage Diff           @@
##               ty     #308   +/-   ##
=======================================
  Coverage   37.58%   37.58%           
=======================================
  Files          18       18           
  Lines        2046     2046           
=======================================
  Hits          769      769           
  Misses       1277     1277           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kklein kklein added the ready label Dec 18, 2025
Copilot AI changed the title [WIP] Fix caching problems in get_factual_value and get_target_value Explain caching implementation issues causing test failures Dec 18, 2025
Copilot AI requested a review from kklein December 18, 2025 23:20
@kklein kklein closed this Dec 19, 2025
@ivergara ivergara deleted the copilot/sub-pr-306 branch December 19, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants