Repository: joblib/joblib
Pull request #1613 (2024-09-27 12:53:49)

minor: enforce age_limit>0 and clarify docs
Hi,

I have been confused by the argument age_limit for some time now.

Here's the current phrasing:
> age_limit: datetime.timedelta, optional
Maximum age of items to limit the cache to. When reducing the size of the cache, any items last accessed more than the given length of time ago are deleted.

So if your mind focuses on the term "age_limit" it's obvious that to remove a >3 day old item you should use age_limit=timedelta(days=3). But if you focus on the term "timedelta" you can easily think relatively to yourself: 'to remove a >3 days old item, I should pick a threshold date that is 3 days into the passed, so age_limit=timedelta(days=-3)'. The fact that this impossible argument is accepted (no cache elements could be in the future) is to me problematic.

# Commit explanations

I decided to add an example in the docstring to illustrate how timedelta should be used to remove cache items that are more than X days old:
- **docs: add example for age_limit in Memory.reduce_size**

I added an assert that forces the timedelta to be positive. As otherwise it is silently ignore as no items are cached in the future (right?):
- **new: enfore positive timedeltas in age_limit of Memory.reduce_size**

I added a test for it:
- **add test for age_limit positivity enforcement**

And modified CHANGES.rst:
- **update changes**


Edit: sort of related to #1488 


Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 683de24..9964856 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -4,6 +4,10 @@ Latest changes
 In development
 --------------
 
+- Enforce ``age_limit`` is a positive timedelta for ``Memory.reduce_size``,
+  to avoid silently ignoring it.
+  https://github.com/joblib/joblib/pull/1613
+
 - Remove deprecated ``bytes_limit`` argument for ``Memory``, which should
   be passed directly to ``Memory.reduce_size``.
   https://github.com/joblib/joblib/pull/1569
diff --git a/joblib/_store_backends.py b/joblib/_store_backends.py
index 68e207c..549b8c0 100644
--- a/joblib/_store_backends.py
+++ b/joblib/_store_backends.py
@@ -348,6 +348,8 @@ class StoreBackendMixin(object):
 
         if age_limit is not None:
             older_item = min(item.last_access for item in items)
+            if age_limit.total_seconds() < 0:
+                raise ValueError("age_limit has to be a positive timedelta")
             deadline = datetime.datetime.now() - age_limit
         else:
             deadline = None
diff --git a/joblib/memory.py b/joblib/memory.py
index 8f2595f..3a64c48 100644
--- a/joblib/memory.py
+++ b/joblib/memory.py
@@ -1078,7 +1078,9 @@ class Memory(Logger):
         age_limit: datetime.timedelta, optional
             Maximum age of items to limit the cache to.  When reducing the size
             of the cache, any items last accessed more than the given length of
-            time ago are deleted.
+            time ago are deleted. Example: to remove files older than 5 days,
+            use datetime.timedelta(days=5). Negative timedelta are not
+            accepted.
         """
         if self.store_backend is None:
             # No cached results, this function does nothing.


Test patch:
diff --git a/joblib/test/test_memory.py b/joblib/test/test_memory.py
index dd401a7..6629d13 100644
--- a/joblib/test/test_memory.py
+++ b/joblib/test/test_memory.py
@@ -1045,6 +1045,10 @@ def test_memory_reduce_size_age_limit(tmpdir):
     assert not set.issubset(set(cache_items), set(ref_cache_items))
     assert len(cache_items) == 2
 
+    # ensure age_limit is forced to be positive
+    with pytest.raises(ValueError, match="has to be a positive"):
+        memory.reduce_size(age_limit=datetime.timedelta(seconds=-1))
+
     # age_limit set so that no cache item is kept
     memory.reduce_size(age_limit=datetime.timedelta(seconds=0))
     cache_items = memory.store_backend.get_items()