Skip to content

Add public method to upfront check if call in cache and valid#1584

Merged
tomMoral merged 6 commits intojoblib:mainfrom
LudwigStumpp:main
Jun 14, 2024
Merged

Add public method to upfront check if call in cache and valid#1584
tomMoral merged 6 commits intojoblib:mainfrom
LudwigStumpp:main

Conversation

@LudwigStumpp
Copy link
Copy Markdown
Contributor

Following the reasoning and implementation behind #820, this PR adds a public method to check if a given cached function call is in cache and valid (=meets the cache validation callback function).

@LudwigStumpp LudwigStumpp force-pushed the main branch 2 times, most recently from 77ce1f4 to 267730b Compare May 18, 2024 22:13
@codecov
Copy link
Copy Markdown

codecov bot commented May 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.27%. Comparing base (dc0cb6d) to head (c623383).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1584      +/-   ##
==========================================
+ Coverage   95.26%   95.27%   +0.01%     
==========================================
  Files          45       45              
  Lines        7705     7708       +3     
==========================================
+ Hits         7340     7344       +4     
+ Misses        365      364       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LudwigStumpp
Copy link
Copy Markdown
Contributor Author

LudwigStumpp commented May 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.22%. Comparing base (dc0cb6d) to head (35bc127).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1584      +/-   ##
==========================================
- Coverage   95.23%   95.22%   -0.02%     
==========================================
  Files          45       45              
  Lines        7705     7725      +20     
==========================================
+ Hits         7338     7356      +18     
- Misses        367      369       +2     

☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

Anyone has a clue where the increase of +2 misses comes from?
EDIT: can this be from the two blank lines after the defined functions?

Copy link
Copy Markdown
Contributor

@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.

Hello,

Thanks for the proposal. I think this is the intent of the check_call_in_cache method.

Instead of adding an extra method, I would avoid duplication and modify the existing method to include checking that the cache is valid.

@LudwigStumpp
Copy link
Copy Markdown
Contributor Author

Hello Tom, yeah, I agree with your proposal of instead improving / fixing the already present check_call_in_cache and will update accordingly.

@LudwigStumpp
Copy link
Copy Markdown
Contributor Author

@tomMoral I just moved the functionality over to the already existing check_call_in_cache method but unfortunately got some errors which to me however seem unrelated to the change I did - however maybe I am missing something. Could you please take a look?

@LudwigStumpp LudwigStumpp requested a review from tomMoral May 24, 2024 20:59
Copy link
Copy Markdown
Contributor

@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.

LGTM!

@tomMoral
Copy link
Copy Markdown
Contributor

it seems that the failure is unrelated, could you push a every in the change log?

LudwigStumpp and others added 2 commits May 31, 2024 19:22
Add name for returned value

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>
@LudwigStumpp
Copy link
Copy Markdown
Contributor Author

@tomMoral I added a section in the change log. Apart from above docs build error that I still seem to be getting, I would be good to go.

Copy link
Copy Markdown
Contributor

@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.

LGTM

@tomMoral tomMoral merged commit 905b0ca into joblib:main Jun 14, 2024
@valankar
Copy link
Copy Markdown

Any idea when this might be released? Just came across the issue thinking check_call_in_cache verifies the cache_validation_callback, but it does not (v1.4.2). Thanks.

@tomMoral
Copy link
Copy Markdown
Contributor

We plan to do a sprint and a release in late February.
Sorry for the delay in the release of this feature.

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.

3 participants