Give Memory.reduce_size() items_limit and age_limit options

[jwodder]

Closes #1183.

Not really sure how to write a test for this; advice appreciated.

[codecov]

Codecov Report

Patch coverage: 98.41% and project coverage change: -0.10 ⚠️

Comparison is base (2303143) 94.90% compared to head (de764ea) 94.81%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1200      +/-   ##
==========================================
- Coverage   94.90%   94.81%   -0.10%     
==========================================
  Files          44       44              
  Lines        7308     7361      +53     
==========================================
+ Hits         6936     6979      +43     
- Misses        372      382      +10     

Impacted Files	Coverage Δ
joblib/memory.py	95.41% <87.50%> (-0.20%)	⬇️
joblib/_store_backends.py	92.01% <100.00%> (+0.43%)	⬆️
joblib/test/test_memory.py	98.55% <100.00%> (+0.07%)	⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tomMoral]

Hello,

Thanks a lot for the PR and sorry for the long delay in the review. I feel this would be a nice addition but not totally sure about the API. Adding all these arguments (we could think of others I am sure) can make it complicated to maintain a simple usage, which is the aim of joblib. I would be in favor of keeping this argument only in Memory.reduce_size.

I did a couple comments and to move forward, I think adding some tests would be required and also adding a simple usage example in the Memory documentation.

[tomMoral]

I am not sure this is a good idea to add this at the class instantiation level as this is not directly enforce by Memory.
This actually makes it confusing in the doc (cf the Note) so maybe a better way would be to directly pass this in reduce_size, so it is clear it is only enforced by calling reduce_size. And if we go with this, maybe it would be a good idea to deprecate bytes_limit.

Not sure about that at all, maybe a better way would be to implement a Thread that checks the limit, but this feels a bit brittle and can add a lot of complexity.

[jwodder]

I have moved these arguments to reduce_size(). I leave it to you to take care of bytes_limit.

[jwodder]

@tomMoral I've made your requested changes (including changing age_limit to a timedelta; not sure why I can't reply to that comment).

I agree that tests should be added; however, I'm not sure how exactly to write them and would appreciate advice.

[tomMoral]

@tomMoral I've made your requested changes (including changing age_limit to a timedelta; not sure why I can't reply to that comment).

Thanks a lot for the modifications.

I agree that tests should be added; however, I'm not sure how exactly to write them and would appreciate advice.

For the tests, I would put some results in cache, then check that they are indeed cached, reduce_size with some harsh limit (age_limits/item_limits=0...) and check that the results are not in cache anymore.
You can use the check_call_in_cache helper to check if a specific call is in cache:

cached_foo = mem.cache(foo)
cached_foo.check_call_in_cache(x)
>>> False
cached_foo(x)
cached_foo.check_call_in_cache(x)
>>> True

[tomMoral]

LGTM, thx for proposing this @jwodder

I just need to add an entry in the change log and we should be good to merge this feature.
I will open another PR to deprecate the bytes_limit from __init__ as this will make clearer that the limits need to be enforced by calling reduce_size manually.