Add LRU caching concept for SeqFetcher and HDP - #877
Conversation
Implement in-process LRU cache decorators for the SeqFetcher and HGVS data provider (HDP) to reduce repeated lookups during validation. The SeqFetcher cache stores repeated fetch_seq() requests. The HDP cache stores results from get_tx_identity_info(), get_tx_for_gene(), get_pro_ac_for_tx_ac(), get_tx_exons(), get_gene_info() and get_tx_mapping_options(). get_tx_for_region() was intentionally excluded because these genomic coordinate queries are unlikely to repeat during validation. Both wrappers use the decorator pattern and transparently delegate all uncached methods to the underlying implementations. Benchmarking shows the SeqFetcher cache primarily improves runtime stability, while the addition of the HDP cache provides a substantial reduction in overall validation time. Refs #876
|
@John-F-Wagstaff. This is a concept, so feel free not to accept. I know you are careful with caches. For reference, taken from the issues link Benchmark methodologyTo evaluate the impact of introducing in-process LRU caches, the full Each benchmark consisted of 2,197 passing tests and 6 skipped tests. Three configurations were compared:
The HDP cache stores the results of repeated calls to:
Results
ConclusionsCaching repeated sequence retrievals alone provides only a modest The majority of the performance improvement comes from caching HGVS Data These results suggest that repeated transcript and annotation lookups Although the combined cache exhibits similar run-to-run variability to |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## regex_removal #877 +/- ##
=================================================
- Coverage 84.84% 84.76% -0.08%
=================================================
Files 48 48
Lines 14554 14601 +47
=================================================
+ Hits 12348 12377 +29
- Misses 2206 2224 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Everything mentioned here, including https://github.com/openvar/vv_hgvs/blob/master/vvhgvs/dataproviders/interface.py Which includes an lru cache for all of this. Yet despite this the benefit of a bit more hits on the cash is enough to make this look like a performance gain, especially for SeqRepo. I currently have no idea what would help for SeqRepo usage, aside from bumping the global cash size. I added TranscriptMapData to help this issue (repeated re-calls of expensive DB fetches), it caches the first fetch it gets for each transcript/transcript-genomic ref pair, for the lifetime of the validation. This works on a per variant basis at the moment. It also currently only affects some I have no idea how much expanding this would help in total VS creating an extra layer of local caching like this PR does, or upping the global cache size however, it did speed things up somewhat when I was testing it, even in circumstances that the global cash should have invalidated the gain. |
|
So the conculusion is I am happy to change the cache sizes etc to see if I can make it even faster. What do you think? |
|
There will be a global removal of a SeqRepo call when a project to map Selenon genes properly concludes in a few weeks and we do the next database build. This will help a lot |
|
Not quite, I have suspicions of why, namely that if you do tests in parallel you might get enough fetches to overflow the cache between "related" calls, causing a re-fetch, the cache size is fixed. When I said the global cache size I was talking about the cache in vv_hgvs, if this cache size is increased it should affect the validation +replace ref calls internal to vvhgvs too. If we get the same performance boost from that then we don't want to add another layer of caching. |
|
We also might see some changes to performance between using Adding an extra layer of caching and a load of extra code to maintain to undo that if it is what is happening would be a waste. |
|
cool so its an in vvta and seqrepo thing. But this shows its worth investigating so was not useless :) |
|
Who knows what happens if we bump the vvhgvs cache size though? it is probably worth checking. The entire original vvhgvs code base was architected by someone who side-loaded a cache of correct answers in before running the tests! He did do some complex checks depending on cache state, but in the state we forked from it effectively defaulted to just testing the existing "pickled" cache unless steps were taken to prevent this. (and he also thought, or possibly still thinks, that even heavy users should fetch from remote servers like everyone else, but just cache harder, from the way thing are written at least. ) If you do want to do a test we need to change |
|
I prefer to do it in vvminininit to make it easier to maintain Setting to 200. Do you want me to delete my local caches or keep them and see what happens, or both? |
|
I don't know what will happen if you change the settings, I don't know whether adding the extra lru cache on top will have any affect at all on performance with an increased underlying cache size. So we have to try the combinations,
|
|
cool can do all this. By the way, pytest or pytest -n 4 for best results that are more realistic? |
|
whatever you did for your first tests for now, I think, so that we don't have to re-do a baseline. |
|
I'm running this now. Just got started, but the results are shocking. Will publish when done |
|
OK, this is really unexpected Final benchmark table (slowest → fastest)
TCaching clearly helps, but whats the sweet spot? |
|
What are the results without the SeqFetcher cache ? We specifically need to know what happens with the vv_hgvs lru cache goes up without any VV local caching VS with or we can't tell wether it is worth adding the VV local caching. The gain from adding the local cache VS running without the local cache should drop as the vv_hgvs lru cache goes up, but how much? We need:
if the difference after a certain point is less than a couple of seconds out of more than 200 that changes things. |
|
Yep, I ran out of time to do the runs with the local hdp cache on, and the local seqfetcher cache on, then both off. Will add these over the weekend. I decided to try max out the gain first to get a ball park, but didn't manage to, but it's all informative and will help get to a conclusion. |
Table 2 – Follow-up benchmarking (evening session)
SummaryThe second benchmarking session investigated the contribution of the HGVS internal LRU cache independently of the custom Local HDP and SeqFetcher wrapper caches. Increasing the HGVS internal cache continued to improve performance from 600 to 1000 entries, with a smaller additional improvement observed at 1200 entries. Disabling both the Local HDP and SeqFetcher caches while retaining an HGVS cache size of 1000 increased the mean runtime to 331.09 s, demonstrating that the wrapper caches contribute additional performance improvements. However, comparison of the remaining configurations indicates that once the HGVS internal cache is increased to 1000–1200 entries, the majority of the performance benefit is attributable to the HGVS internal cache itself. Within this benchmark session, the fastest configuration was:
This achieved a mean runtime of 277.66 s, corresponding to a 40.16% reduction relative to the uncached baseline measured in Session 1. The difference between HGVS LRU = 1000 and HGVS LRU = 1200 was modest (281.66 s versus 277.66 s), indicating diminishing returns as the cache size increases. Based on these data, a cache size of approximately 1000–1200 entries per cached HGVS method appears to capture most of the achievable performance benefit for the VariantValidator functional test suite. The benchmark also suggests that the additional Local HDP wrapper cache provides little or no measurable benefit once the HGVS internal cache has been increased to this range. |
Configure the benchmark branch with the current optimal cache configuration identified during performance testing. Changes: - Increase the HGVS internal LRU cache size to 1000 entries. - Leave the SeqFetcher cache enabled. - Disable the Local HDP cache wrapper. - Retain the Local HDP cache implementation in the codebase for future benchmarking and evaluation. Benchmarking indicates that an HGVS LRU cache size of 1000 provides the best balance between execution speed and expected memory usage for the current VariantValidator workload. Increasing the cache beyond 1000 yielded only marginal additional performance improvements.
|
So if we intend to bump the cache to 1.2k or so we just want to boost the vvhgvs cache setting, and not pull this? I should note that since the tests end up repeatedly target the same problem transcripts in different ways, we can not expect the same level of performance gain for user input as we get for tests (from either cache version), though some specific batch tests may gain a outsized benefit. |
SeqFetcher cache benchmarking (HGVS LRU = 1000, Local HDP cache disabled)
SummaryThe final phase of benchmarking evaluated the impact of the SeqFetcher LRU cache while keeping the HGVS internal LRU cache fixed at 1000 entries and the Local HDP cache disabled. Enabling the SeqFetcher cache immediately produced a substantial improvement in execution time, reducing the runtime by approximately 15% compared with having no SeqFetcher cache. Increasing the cache size beyond 4096 continued to improve performance, although the magnitude of the improvement steadily decreased, demonstrating the expected diminishing returns of an LRU cache. Performance continued to improve through 8192, 16384, 32768, and 65536 entries, with the largest gains achieved at the smaller cache sizes and progressively smaller improvements thereafter. The fastest configuration tested was 65536 entries, achieving an 18.78% reduction in runtime relative to the uncached configuration. However, the improvement over 32768 entries was relatively modest (approximately 2.8 s, or 0.9–1.1%), suggesting that the cache is approaching saturation for the VariantValidator functional test workload. Overall, these benchmarks indicate that the SeqFetcher cache provides a significant performance benefit, while increasing the cache beyond 32768 entries yields only marginal additional improvement. Consequently, 32768 entries remains a sensible default configuration, providing near-optimal performance while avoiding unnecessary growth in cache size. Agreed. We should not pull this. I will make a fresh cleaned branch, but wanted to add this in before doing it. I think the sf cache is optimal already too |
SummaryThe final benchmarking demonstrates that the combined caching strategy provides a substantial improvement in VariantValidator performance. With all caches disabled, the complete test suite required an average of 26.58 minutes. Enabling the recommended caching configuration reduced this to 4.19 minutes, representing an overall reduction of 1343.06 seconds (22.38 minutes), or an 84.2% decrease in execution time. The recommended cache configuration is therefore:
Further increases in cache size did not produce measurable improvements, indicating that these settings provide a good balance between execution speed and memory consumption for the VariantValidator validation workload. |
I agree. The test suite is something of a best-case scenario because it repeatedly validates the same genes and transcripts, allowing the caches to warm up and be reused. We therefore should not expect the same magnitude of improvement for a single user validation. That said, I think there are several important benefits: Individual validations should still improve because a single validation performs multiple repeated SeqFetcher, database and VVTA/HDP lookups internally, so there are opportunities for cache hits even within a single request. Finally, one aspect I think is important is that this work was benchmark-driven rather than assumption-driven. We didn't simply add caches everywhere. Each cache was benchmarked, cache sizes were tuned empirically, and caches that showed little or no measurable benefit were either reduced to their optimum size or deliberately not implemented. The result is a caching strategy that balances execution speed, memory usage and maintainability, rather than simply maximising cache size or caching every possible lookup. |
|
An additional point worth noting is that the comparison against the last develop branch is conservative. Since that point, the project has gained a substantial number of new unit and regression tests, increasing the overall workload of the CI pipeline. Despite this, the end-to-end GitHub Actions runtime has reduced from approximately 36 minutes on develop to approximately 22 minutes on the final optimisation branch. This represents an improvement of almost 14 minutes (≈38%), even though the pipeline is now executing a larger and more comprehensive test suite. In other words, the optimisation has not simply made the previous workload faster—it has made a larger workload complete in significantly less time. That improvement is the cumulative result of the codebase clean-up, removal of unnecessary regex and string processing, increased use of HGVS objects, reduced object creation, improved lookup paths, targeted caching, and benchmark-driven cache tuning. Together, these changes have improved maintainability, increased automated test coverage, and substantially reduced execution time for both local development and continuous integration. I strongly belive that when you add your own optimisations @John-F-Wagstaff h, i.e. the fine-tooth-comb clean, we have done a really good job of this refactor. Also, test coverage is now just marginally below 85%. |
|
The very fact that the seqFetcher cache produces performance results like this however means that we are probably doing an end-run around the hdp inbuilt caching for sequence fetch. Otherwise the difference between no caching and the first cached test should probably be a lot smaller, though I would be more certain if you had set the cache size to 1000 for the first test not 4 times that. This does allow us to have a higher seq fetch cache than the for the other endpoints though. Despite this should probably see a better results for the same cache size if the vvhgvs cache is also used by the VV, so in future we may want to add the provision to have a different vvhgvs internal cache size for seq fetch than the other endpoints, fix the accidental end run, and then remove the extra seq fetcher cache code from VV to avoid the unneeded extra complexity. |
2 participants
Implement in-process LRU cache decorators for the SeqFetcher and HGVS data provider (HDP) to reduce repeated lookups during validation.
The SeqFetcher cache stores repeated fetch_seq() requests. The HDP cache stores results from get_tx_identity_info(), get_tx_for_gene(), get_pro_ac_for_tx_ac(), get_tx_exons(), get_gene_info() and get_tx_mapping_options(). get_tx_for_region() was intentionally excluded because these genomic coordinate queries are unlikely to repeat during validation.
Both wrappers use the decorator pattern and transparently delegate all uncached methods to the underlying implementations.
Benchmarking shows the SeqFetcher cache primarily improves runtime stability, while the addition of the HDP cache provides a substantial reduction in overall validation time.
Refs #876