MAINT: Refactor caching (#124, #148, #153) #219
Conversation
- replace `@cached_var` with `@functools.lru_cache`
and `@cached_const` with `@functools.cached_property`
- refactor calls of any `cached_property`, now not callable anymore,
substitute with `self.set_link_attribute(property)` where necessary
- remove `cache_helper` and `cache` attribute in `core.network`
- adapt `clear_cache()` methods accordingly,
introduce `clear_cache_startswith()`
- remove `Network.clear_link_attribute()`
- adapt `Network.{del|set}_link_attribute()` accordingly,
drop clearing cache of `path_lengths()` in `set_link_attribute()`
- related issue: #148
- locally disable pylint false positive `not-callable` on
`multiprocess.Pool()`
- replace removed `clear_paths_cache()`
with new `clear_cache_startswith('path_')`
- related: #148
This reverts commit fe23f23.
This reverts commit 0166cd2.
Improve the approach in reverted commits fe23f23, 0166cd2: - introduce `@Cached.method()` based on `@functools.lru_cache()` - track mutable dependencies for cache invalidation - fix cache clearing helper - add behavioural spec in `tests/test_core/test_cache.py` Adjustments in existing classes in this commit: - define `__cache_state__()` for all subclasses of `Cached` (might require updates when adding further methods to the cache) - define `@Cached.method(attrs=(...))` where appropriate - remove obsolete class-specific caching mechanisms - factor out cached worker: `Network.nsi_betweenness()` - define helper: `Network.find_link_attribute()` - define helper: `ClimateNetwork._weighted_metric()` Adding this behaviour to classes without caching should be straightforward. Classes that remain to be adjusted (still have a `clear_cache()` method or subclass `Cached` without conforming to its protocoll): - `ClimateNetwork` and its subclasses: The recursive interaction between `__init__()`, `_similarity_measure` and `_regenerate_network()` across the class hierarchy is very stateful and does not fit into the regular OO dependency pattern assumed by `Cached`. A redesign of this logic would be advisable, but is left for future work. - `RecurrenceNetwork` and its subclasses, and `Surrogate`: Left as an exercise for the reader.
- fix indentation - add missing reference
- importing `Hashable` and `Callable` from `collections.abc` directly - thereby fix pylint warnings from 93ab748
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 62.82% 63.76% +0.93%
==========================================
Files 43 44 +1
Lines 6220 6221 +1
==========================================
+ Hits 3908 3967 +59
+ Misses 2312 2254 -58 ☔ View full report in Codecov by Sentry. |
|
Thanks a lot @ntfrgl for all the work put into implementing this and also for all the little cleanups and refactors along the way! I took a bit of time to understand the structure and functionality of the new I added the remaining minor fixes in the commits above to make sure all checks pass, so this PR should be good to go. |
|
Thank you for the careful review. I'm glad to know the project in your hands. As a last addition, I slightly simplified the implementation of nested |
This PR fully implements:
Cachedwith a decorator@Cached.method(),Obviously, there is still more refactoring to be done to make the caching system consistent across the project, but this PR should deliver on the main concern of #148, namely a well-behaved caching decorator. The last commit message should provide enough context for reviewing. @fkuehlein, please raise any concerns you might have, since you are expected to maintain this code going forward.