Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH add cache_validation_callback in Memory #1149

Merged
merged 21 commits into from Jun 22, 2023

Conversation

tomMoral
Copy link
Contributor

@tomMoral tomMoral commented Jan 11, 2021

Tentative to implement a way to validate the cache results with user define rules.

  • Add an argument validate_cache that takes a callable validate_cache(metadata) -> bool.
  • Add time in metadata to ease the robust checking of computation time in all backends.
  • Add an helper joblib.memory.expires_after to check the age of a cached results.
  • Add some small tests
  • Clean some leftover pep8 :)

Todo:

  • Add some doc if the API is validated.
  • Add some entry in changelog.

Fixes #313 .

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Patch coverage: 98.82% and project coverage change: +0.10 🎉

Comparison is base (9f7edc2) 94.82% compared to head (c06dc88) 94.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
+ Coverage   94.82%   94.93%   +0.10%     
==========================================
  Files          44       44              
  Lines        7366     7439      +73     
==========================================
+ Hits         6985     7062      +77     
+ Misses        381      377       -4     
Impacted Files Coverage Δ
joblib/memory.py 95.42% <96.77%> (+0.23%) ⬆️
joblib/__init__.py 100.00% <100.00%> (ø)
joblib/test/test_memory.py 98.52% <100.00%> (+0.09%) ⬆️

... and 1 file 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.

Copy link
Contributor Author

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some improvements

joblib/memory.py Show resolved Hide resolved
joblib/memory.py Show resolved Hide resolved
joblib/memory.py Show resolved Hide resolved
joblib/test/test_memory.py Outdated Show resolved Hide resolved
joblib/test/test_memory.py Outdated Show resolved Hide resolved
joblib/memory.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tomMoral,

After some delay, here is a first superficial pass. I still need to get into MemorizedFunc's internals to see how everything fits together.

joblib/memory.py Outdated Show resolved Hide resolved
joblib/memory.py Outdated Show resolved Hide resolved
joblib/memory.py Show resolved Hide resolved
joblib/memory.py Outdated Show resolved Hide resolved
joblib/memory.py Outdated Show resolved Hide resolved
joblib/test/test_memory.py Outdated Show resolved Hide resolved
joblib/test/test_memory.py Outdated Show resolved Hide resolved
joblib/test/test_memory.py Outdated Show resolved Hide resolved
joblib/test/test_memory.py Outdated Show resolved Hide resolved
joblib/memory.py Show resolved Hide resolved
@tomMoral
Copy link
Contributor Author

Thx for the review @jjerphan ! I did update this a bit. I will try to finish this by adding some examples and a bit of doc.

@jjerphan
Copy link
Contributor

Thx for the review @jjerphan !

You're welcome.

I did update this a bit. I will try to finish this by adding some examples and a bit of doc.

Feel free to request a new review when it's ready.

@tomMoral
Copy link
Contributor Author

@jjerphan I rebased the PR and did a pass on the doc, feel free to review once more :)

@jjerphan
Copy link
Contributor

It looks like the tests are failing in some configurations.

I mostly am busy with other tasks now but will try to come back to it soon.

joblib/memory.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing a few remark and changing this PR title to mention cache_validation_callback instead of validate_cache, this LGTM. 👍

doc/memory.rst Outdated Show resolved Hide resolved
doc/memory.rst Outdated Show resolved Hide resolved
doc/memory.rst Show resolved Hide resolved
doc/memory.rst Outdated Show resolved Hide resolved
doc/memory.rst Show resolved Hide resolved
doc/memory.rst Outdated Show resolved Hide resolved
joblib/__init__.py Outdated Show resolved Hide resolved
@tomMoral tomMoral changed the title ENH add validate_cache in Memory ENH add cache_validation_callback in Memory Jan 3, 2023
tomMoral and others added 2 commits January 3, 2023 12:19
@jjerphan
Copy link
Contributor

jjerphan commented Jan 3, 2023

My previous review might be outdated, let me know if you want another one, @tomMoral.

@xloem
Copy link

xloem commented May 27, 2023

This popular feature appears to have rested unmerged after passing an extended review. Sadly there are now conflicts and test failures again. Is this good to merge if it is ported forward?

@xloem xloem mentioned this pull request May 27, 2023
joblib/memory.py Outdated Show resolved Hide resolved
@tomMoral
Copy link
Contributor Author

I just fixed this, I will try to merge it to include it in the next release.

@tomMoral tomMoral merged commit b469022 into joblib:master Jun 22, 2023
16 checks passed
@tomMoral tomMoral deleted the ENH_valid_cache branch June 22, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expiry of old results
3 participants