Improving class loading efficiency and considering dynamic type list #919
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n new code after underline changes related to data pack. After discussed with Hector using standard LRU_Cache instead of custom cache.
… created an non-cached version of get_class (get_class_nc) and used in get_class to avoid dynamic list issue as mentioned in comments of asyml#765 as pointed out my Hector.
|
please fix the "This PR fixes" line again. Please learn how to use this syntax |
hunterhector
reviewed
Feb 1, 2023
Sorry forgot to remove the "[ ]" in the template. The format is now fixed. |
…at have optional list as parameter -- change to cache lower level result from "locate" method called in this method, and also provide a non-cached version (same as before) for resolving asyml#765.
Codecov Report
@@ Coverage Diff @@
## master #919 +/- ##
=======================================
Coverage 81.05% 81.06%
=======================================
Files 256 256
Lines 19851 19858 +7
=======================================
+ Hits 16091 16097 +6
- Misses 3760 3761 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
hunterhector
reviewed
Apr 2, 2023
…e-factoring into one method with "cached_lookup" optional flag -- by default this is true but in _get_sub_class this flag needs to set to "false" based on comments in asyml#765 (for dynamic class loading scenario).
hunterhector
approved these changes
Apr 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #908.
This PR addresses class loading efficiency issue by adding lru_cache to get_class() method, and also resolved the concern raised in #765 about dynamic type list indata_store, so in the "is_subclass" method a special non-cached version of get_class (get_class_nc) is called instead
Possible influences of this PR.
By using a special non-cached version of get_class (get_class_nc) in "is_subclass" might cause a slight performance impact of a few percentage (to ensure correctness), in the future there might be improved ways to make the method using some kind of caching mechanism safely.
Test Conducted
testPOSTaggingNER() method in "data_pack_profiling_test" for checking performance changes (performance improved from non-lru cached versions of "get_class" in utils.py)