✨(api) change cache key to indicator attributes

Previously, variations in indicator attributes such as boolean flags or environment settings led to divergent results for identical LRS queries. We could not use cache mixins in the cases, as we only rely on the LRS query as a cache key. Changing the cache key to be a hash from the indicator instance attributes, so that cache mixins are usable for these indicators.
openfun · May 16, 2024 · ebcbda2 · ebcbda2
1 parent 9341e6d
commit ebcbda2
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 54 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ and this project adheres to
 ### Changed
 
 - Restrict dashboards access to instructors and administrators
+- Change cache key to indicator attributes instead of LRS query parameters
 
 ### Fixed
 

diff --git a/src/api/core/warren/indicators/mixins.py b/src/api/core/warren/indicators/mixins.py
@@ -2,6 +2,7 @@
 
 import hashlib
 import inspect
+import json
 import logging
 from abc import ABC, abstractmethod
 from functools import cached_property, reduce
@@ -44,10 +45,6 @@
 class Cacheable(Protocol):
     """A protocol defining the structure for cacheable objects."""
 
-    def get_lrs_query(self):
-        """Get the LRS query for fetching statements."""
-        ...
-
     async def compute(self):
         """Perform operations to get a computed value."""
         ...
@@ -67,17 +64,17 @@ def db_session(self) -> Session:
     def cache_key(self) -> str:
         """Calculate the indicator cache key.
 
-        The cache key is composed of the class name and a hexadecimal digest
-        of the LRS query hash (excluding the since and until fields if present),
-        e.g. fooindicator-44709d6fcb83d92a76dcb0b668c98e1b1d3dafe7.
+        The cache key is composed of the class name and a hexadecimal digest of the
+        instance attributes, e.g.
+        fooindicator-44709d6fcb83d92a76dcb0b668c98e1b1d3dafe7.
+
+        Note that instance attributes need to be serializable, or at least implement the
+        __str__ method.
 
         """
-        lrs_query_hash = hashlib.sha256(
-            self.get_lrs_query()
-            .json(exclude={"since", "until"}, exclude_none=True, exclude_unset=True)
-            .encode()
-        ).hexdigest()
-        return f"{self.__class__.__name__.lower()}-{lrs_query_hash}"
+        attributes = json.dumps(vars(self), sort_keys=True, default=str)
+        attributes_hash = hashlib.sha256(attributes.encode()).hexdigest()
+        return f"{self.__class__.__name__.lower()}-{attributes_hash}"
 
     async def get_cache(self) -> Union[CacheEntry, None]:
         """Get cached results matching the cache key from the database."""
@@ -180,6 +177,21 @@ class IncrementalCacheMixin(CacheMixin, CacheableIncrementally, ABC):
 
     frame: Frames
 
+    @cached_property
+    def cache_key(self) -> str:
+        """Calculate the indicator cache key.
+
+        The cache key is composed of the class name and a hexadecimal digest
+        of the class instance attributes (excluding the span_range field if present),
+        e.g. fooindicator-44709d6fcb83d92a76dcb0b668c98e1b1d3dafe7.
+
+        """
+        attr_copy = vars(self).copy()
+        attr_copy.pop("span_range", None)
+        attributes = json.dumps(attr_copy, sort_keys=True, default=str)
+        attributes_hash = hashlib.sha256(attributes.encode()).hexdigest()
+        return f"{self.__class__.__name__.lower()}-{attributes_hash}"
+
     @staticmethod
     @abstractmethod
     def merge(a: Any, b: Any) -> Any:

diff --git a/src/api/core/warren/tests/indicators/test_mixins.py b/src/api/core/warren/tests/indicators/test_mixins.py
@@ -28,8 +28,10 @@ def test_cache_key_calculation():
     class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
-        def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+        course_id: str
+
+        def get_lrs_query(self):
+            pass
 
         async def fetch_statements(self):
             pass
@@ -38,39 +40,26 @@ async def compute(self):
             pass
 
     indicator = MyIndicator()
-    lrs_query = '{"verb": "https://w3id.org/xapi/video/verbs/played"}'
-    expected = f"myindicator-{hashlib.sha256(lrs_query.encode()).hexdigest()}"
+    attributes = '{"span_range": null}'
+    expected = f"myindicator-{hashlib.sha256(attributes.encode()).hexdigest()}"
     assert indicator.cache_key == expected
 
+    indicator = MyIndicator(course_id="test_id")
+    attributes = '{"course_id": "test_id", "span_range": null}'
+    expected = f"myindicator-{hashlib.sha256(attributes.encode()).hexdigest()}"
+    assert indicator.cache_key == expected
 
-def test_cache_key_calculation_with_lrs_query_datetime_range():
-    """Test the cache key calculation when the LRS query as date time range
-    (since/until) parameters set.
-
-    """  # noqa: D205
-
-    class MyIndicator(BaseIndicator, CacheMixin):
-        """Dummy indicator."""
-
-        def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(
-                verb="https://w3id.org/xapi/video/verbs/played",
-                since=datetime(2023, 1, 1),
-                until=datetime(2023, 2, 1),
-            )
-
-        async def fetch_statements(self):
-            pass
-
-        async def compute(self):
-            pass
-
-    indicator = MyIndicator()
-    # We exclude "since" and "until" parameters from the LRS query to calculate
-    # the cache key
-    lrs_query = '{"verb": "https://w3id.org/xapi/video/verbs/played"}'
-    expected = f"myindicator-{hashlib.sha256(lrs_query.encode()).hexdigest()}"
+    span_range = DatetimeRange(since="2023-01-01 10:42", until="2023-01-02 12:22")
+    indicator = MyIndicator(course_id="test_id", span_range=span_range)
+    attributes = (
+        '{"course_id": "test_id", '
+        '"span_range": "since=datetime.datetime(2023, 1, 1, 10, 42, tzinfo=tzutc()) '
+        'until=datetime.datetime(2023, 1, 2, 12, 22, tzinfo=tzutc())"}'
+    )
+    expected = f"myindicator-{hashlib.sha256(attributes.encode()).hexdigest()}"
     assert indicator.cache_key == expected
+    indicator2 = MyIndicator(span_range=span_range, course_id="test_id")
+    assert indicator2.cache_key == expected
 
 
 @pytest.mark.anyio
@@ -82,7 +71,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -117,7 +106,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -162,7 +151,7 @@ class MyIndicatorA(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -174,7 +163,7 @@ class MyIndicatorB(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -205,7 +194,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -230,7 +219,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -258,7 +247,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return None
+            pass
 
         async def fetch_statements(self):
             pass
@@ -364,7 +353,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -393,7 +382,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy mocked indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass
@@ -439,7 +428,7 @@ class MyIndicator(BaseIndicator, CacheMixin):
         """Dummy indicator."""
 
         def get_lrs_query(self) -> LRSStatementsQuery:
-            return LRSStatementsQuery(verb="https://w3id.org/xapi/video/verbs/played")
+            pass
 
         async def fetch_statements(self):
             pass