GSK-1911 Improve performance of the push feature & make CTA almost instant #1512

Googleton · 2023-10-26T13:00:34Z

Description

Includes a "SimpleCache" class that uses hashlib to cache anything we want inside it. It only stores the last 20 results.

This greatly improves the usability of the insights feature by:

Due to being the last-seen example, all calls to CTA will be cached making them much faster
Since we store a few examples, if I'm navigating between a few examples to do comparisons, we always hit the cache and end up with ultra fast results

It also includes a small rewording that JM asked me to do, because it's only text changes I did not want to create yet another PR for it.

…stant

linear · 2023-10-26T13:00:46Z

GSK-1911 CTA add test takes too long (15 sec, just to show all the test suite names)

Screen Recording 2023-10-19 at 14.23.38.mov

Hartorn

You need to write some unit tests on you new class.
Also, I would prefer the caching to be done with a parametrize decorator, to be easily re-usable.
I think you can add this behaviour to websocket_actor, with a new parameter "cache" defaulting to false, using params as the object to hash

Hartorn · 2023-10-26T13:12:41Z

giskard/ml_worker/utils/cache.py

+
+    def add_result(self, obj, result):
+        # Calculate the hash of the object
+        obj_hash = hashlib.md5(repr(obj).encode()).hexdigest()


Can you add encoding explicitely to encode pls ?

Hartorn · 2023-10-26T13:13:12Z

giskard/ml_worker/utils/cache.py

+
+class SimpleCache:
+    def __init__(self, max_results=20):
+        self.results = {}


Could you add typing hint here, for better readability ?

Hartorn · 2023-10-26T13:14:58Z

giskard/ml_worker/utils/cache.py

+        self.results[obj_hash] = result
+
+        # Add the object's hash to the order list and remove the oldest result if necessary
+        self.order.append(obj_hash)


I think you should use LRU (least recently used) algorithm here, instead of oldest.

Hartorn · 2023-10-26T13:15:31Z

giskard/ml_worker/websocket/listener.py

@@ -54,11 +55,9 @@
 from giskard.utils.analytics_collector import analytics

 logger = logging.getLogger(__name__)
-
-
+push_cache = SimpleCache(max_results=20)


Could you write it as PUSH_CACHE instead ?

Also, 20 is not enough I think. Maybe 128 ?

Hartorn · 2023-10-26T13:16:28Z

giskard/ml_worker/websocket/listener.py

-    perturbs = create_perturbation_push(model, dataset, df)
-    overconf = create_overconfidence_push(model, dataset, df)
-    borderl = create_borderline_push(model, dataset, df)
+    cached_result_tuple = push_cache.get_result(params)


Why not using it on the overall get_push instead ?

Hartorn · 2023-10-26T13:21:11Z

You need to write some unit tests on you new class. Also, I would prefer the caching to be done with a parametrize decorator, to be easily re-usable. I think you can add this behaviour to websocket_actor, with a new parameter "cache" defaulting to false, using params as the object to hash

And also, please try to use type hinting for variable and methods