Skip to content

Replace manual cache with functools lru-cache - #1696

Open
linsword13 wants to merge 1 commit into
GoogleCloudPlatform:developfrom
linsword13:lru-cache
Open

Replace manual cache with functools lru-cache#1696
linsword13 wants to merge 1 commit into
GoogleCloudPlatform:developfrom
linsword13:lru-cache

Conversation

@linsword13

Copy link
Copy Markdown
Member

This has the benefit of restricting unbounded cache growth, and improves on readability, without having the manual cache implementation cluttering the main logic.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors caching mechanisms across several modules in Ramble to use Python's built-in functools.lru_cache instead of custom dictionary-based caches. Specifically, it updates _ast_parse in expander.py, limits the cache size of _parse_when in language_helpers.py, and converts repository and workspace file-reading caches to static methods decorated with lru_cache. Feedback on these changes highlights a critical issue in workspace.py: converting read_file_content to a static method with a global lru_cache can lead to global cache pollution across unit tests and incorrect behavior if relative paths are resolved under different working directories. Reverting to an instance-specific cache is recommended.

Comment thread lib/ramble/ramble/workspace/workspace.py Outdated
@ramble-pr-bot

ramble-pr-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

Ramble Performance Test Metrics

Results produced with commit: c28c192

Test Name Outcome Duration (s) Most Recent Run (s) Last 5 Avg (s)
test_analyze_large_file passed 2.9997 2.0820 (b1def73) 2.8854
test_large_template_expansion passed 2.4655 1.4817 (b1def73) 1.9445
test_many_experiments passed 35.0730 28.5715 (b1def73) 32.5937
test_many_objects_defaults passed 21.5774 16.9454 (b1def73) 20.3676
test_matrix_filter_perf passed 1.6812 1.0641 (b1def73) 1.2716

This has the benefit of restricting unbounded cache growth, and improves on readability, without having the manual cache implementation cluttering the main logic.
@linsword13

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces manual caching mechanisms with functools.lru_cache to optimize memory usage and simplify the codebase. Specifically, it updates _ast_parse in expander.py to use an LRU cache with a maximum size of 2048, limits the cache size of _parse_when in language_helpers.py to 4096, and refactors FastObjectChecker in repository.py to use a cached static method instead of a manual global dictionary cache. There are no review comments, and I have no additional feedback to provide.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.33%. Comparing base (b1def73) to head (c28c192).

Files with missing lines Patch % Lines
lib/ramble/ramble/repository.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1696      +/-   ##
===========================================
- Coverage    93.33%   93.33%   -0.01%     
===========================================
  Files          364      364              
  Lines        35481    35476       -5     
===========================================
- Hits         33116    33111       -5     
  Misses        2365     2365              

☔ View full report in Codecov by Harness.
📢 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant