Give Memory.reduce_size() items_limit and age_limit options#1200
Give Memory.reduce_size() items_limit and age_limit options#1200tomMoral merged 13 commits intojoblib:masterfrom jwodder:gh-1183
items_limit and age_limit options#1200Conversation
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
files_limit optionfiles_limit and age_limit options
tomMoral
left a comment
There was a problem hiding this comment.
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.
joblib/memory.py
Outdated
| def __init__(self, location=None, backend='local', cachedir=None, | ||
| mmap_mode=None, compress=False, verbose=1, bytes_limit=None, | ||
| backend_options=None): | ||
| files_limit=None, age_limit=None, backend_options=None): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have moved these arguments to reduce_size(). I leave it to you to take care of bytes_limit.
|
@tomMoral I've made your requested changes (including changing I agree that tests should be added; however, I'm not sure how exactly to write them and would appreciate advice. |
files_limit and age_limit optionsitems_limit and age_limit options
Thanks a lot for the modifications.
For the tests, I would put some results in cache, then check that they are indeed cached, 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
left a comment
There was a problem hiding this comment.
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.
Closes #1183.
Not really sure how to write a test for this; advice appreciated.