chore: use functools.cached_property for cached properties
#4187
Conversation
`functools.cached_property` (new in Python 3.8) is more suitable for cached properties.
Signed-off-by: Jinzhe Zeng <[email protected]>
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe pull request introduces several modifications across three files, focusing on enhancing property management through the use of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
deepmd/tf/utils/tabulate.py (3)
Line range hint
779-792: Consider usingfunctools.cachefor_all_excludedmethod.While the change to use
@cached_propertyfor_n_all_excludedis good, we could further improve the caching strategy for the_all_excludedmethod.Suggestion:
Replace@lru_cachewith@functools.cachefor the_all_excludedmethod.@functools.cacheis a simpler unbounded cache introduced in Python 3.9, which is more appropriate for this use case as it doesn't require specifying a max size.from functools import cache @cache def _all_excluded(self, ii: int) -> bool: """Check if type ii excludes all types.""" return all((ii, type_i) in self.exclude_types for type_i in range(self.ntypes))This change would:
- Simplify the caching mechanism
- Potentially improve performance by removing the need for LRU tracking
- Make the code more consistent with the use of modern Python features
Line range hint
1-892: Consider applying caching to other methods for performance optimization.While the current changes improve the caching strategy for
_n_all_excluded, there are other methods in the class that might benefit from similar optimizations. Consider applying@cached_propertyor@functools.cacheto methods that:
- Have deterministic output based solely on their inputs
- Are called frequently
- Have computationally expensive operations
Potential candidates for optimization:
_get_layer_size_get_table_size_get_data_type_get_last_layer_sizeExample:
@cached_property def _layer_size(self): # Existing implementation of _get_layer_size ... @cached_property def _table_size(self): # Existing implementation of _get_table_size ...This refactoring could potentially improve the overall performance of the class by reducing redundant computations.
Line range hint
1-892: Overall good changes, but consider a more comprehensive optimization pass.The current changes to improve caching for the
_n_all_excludedmethod are a step in the right direction. However, given the complexity of theDPTabulateclass and its critical role in model compression, it might be beneficial to conduct a more comprehensive optimization pass.Suggestions for future improvements:
- Consistently apply modern caching strategies (
@cached_property,@functools.cache) throughout the class where appropriate.- Consider refactoring large methods (e.g.,
build,_build_lower) into smaller, more manageable functions.- Evaluate the use of NumPy operations for performance-critical sections, especially in methods like
_make_data.- Add more comprehensive type hints to improve code readability and catch potential type-related issues early.
- Consider adding more detailed docstrings for complex methods to improve maintainability.
These changes could significantly improve the performance, readability, and maintainability of this critical component. Would you like assistance in planning or implementing any of these suggestions?
deepmd/tf/infer/deep_eval.py (4)
Line range hint
308-313: Ensure proper management of cached TensorFlow sessionWhile caching the
sessproperty using@cached_propertyavoids re-creating the TensorFlow session multiple times, it's important to ensure that the session is properly closed when it's no longer needed to prevent resource leaks.Consider adding a method to explicitly close the session when the
DeepEvalinstance is no longer in use or implementing context management to handle the session's lifecycle.
Line range hint
340-340: Correct method name spelling:_graph_compatable→_graph_compatibleThere is a typo in the method name
_graph_compatable. The correct spelling should be_graph_compatible.Apply this diff to correct the typo:
-def _graph_compatable(self) -> bool: +def _graph_compatible(self) -> bool:
Line range hint
1220-1224: Ensure proper management of cached TensorFlow session inDeepEvalOldCaching the
sessproperty inDeepEvalOldusing@cached_propertyis efficient but requires careful management to prevent resource leaks. Ensure that the TensorFlow session is properly closed when the instance is no longer in use.Consider implementing a mechanism to manage the session's lifecycle, such as a method to close the session explicitly or using a context manager.
1220-1220: Correct method name spelling inDeepEvalOld:_graph_compatable→_graph_compatibleThere is a typo in the method name
_graph_compatablein theDeepEvalOldclass. It should be corrected to_graph_compatible.Apply this diff to correct the typo:
-def _graph_compatable(self) -> bool: +def _graph_compatible(self) -> bool:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- deepmd/tf/infer/deep_eval.py (7 hunks)
- deepmd/tf/utils/tabulate.py (2 hunks)
- deepmd/utils/data_system.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (8)
deepmd/tf/utils/tabulate.py (1)
774-777: Improved caching strategy for_n_all_excludedmethod.The change from
@property @lru_cacheto@cached_propertyfor the_n_all_excludedmethod is a good optimization.@cached_propertycombines the functionality of@propertyand@lru_cache(maxsize=None), providing a more efficient and cleaner implementation.Benefits of this change:
- Reduced memory usage:
@cached_propertystores the result directly on the instance, unlike@lru_cachewhich maintains a separate cache.- Thread-safety:
@cached_propertyis thread-safe by default, which is beneficial in multi-threaded environments.- Simplified code: The decorator combines two decorators into one, making the code more readable.
deepmd/utils/data_system.py (2)
6-6: Confirmcached_propertycompatibility with project requirementsThe use of
functools.cached_propertyrequires Python 3.8 or higher. Please ensure that the project's minimum Python version is 3.8 or newer to maintain compatibility.
Line range hint
241-246: Efficient caching with@cached_propertyfordefault_meshUtilizing
@cached_propertyfor thedefault_meshproperty effectively caches its value, enhancing performance by avoiding unnecessary recomputations.deepmd/tf/infer/deep_eval.py (5)
4-4: Use offunctools.cached_propertyis appropriateThe import of
cached_propertyfrom thefunctoolsmodule allows the use of the@cached_propertydecorator, which efficiently caches property values. This change aligns with the PR objective to utilizefunctools.cached_propertyfor cached properties.
Line range hint
266-288: Appropriate use of@cached_propertyformodel_typeDecorating the
model_typemethod with@cached_propertyis appropriate since the model type is a constant attribute that does not change during the lifetime of theDeepEvalinstance. Caching it improves performance by preventing redundant computations.
Line range hint
290-306: Appropriate use of@cached_propertyformodel_versionApplying
@cached_propertyto themodel_versionmethod is suitable because the model version remains constant. This change enhances efficiency by caching the value after the first retrieval.
Line range hint
1192-1200: Appropriate use of@cached_propertyformodel_typeinDeepEvalOldDecorating the
model_typemethod with@cached_propertyin theDeepEvalOldclass is acceptable since the model type remains constant. This change improves performance by caching the value.
Line range hint
1202-1218: Appropriate use of@cached_propertyformodel_versioninDeepEvalOldApplying
@cached_propertyto themodel_versionmethod inDeepEvalOldis appropriate. Caching this property enhances efficiency by avoiding repeated computations of a constant value.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4187 +/- ##
==========================================
- Coverage 83.45% 83.45% -0.01%
==========================================
Files 537 537
Lines 52148 52140 -8
Branches 3047 3047
==========================================
- Hits 43519 43511 -8
Misses 7683 7683
Partials 946 946 ☔ View full report in Codecov by Sentry. |
functools.cached_property(new in Python 3.8) is more suitable for cached properties.Summary by CodeRabbit
New Features
neighbor_listfor enhanced neighbor list handling in model evaluation.test_sizeparameter for flexible test size configuration._make_auto_tsto facilitate test size calculations based on specified percentages.Bug Fixes
Documentation