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 allow caching coroutine functions #894
Conversation
Codecov Report
@@ Coverage Diff @@
## master #894 +/- ##
==========================================
- Coverage 95.15% 92.09% -3.07%
==========================================
Files 45 47 +2
Lines 6440 6549 +109
==========================================
- Hits 6128 6031 -97
- Misses 312 518 +206
Continue to review full report at Codecov.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #894 +/- ##
=======================================
Coverage ? 95.21%
=======================================
Files ? 45
Lines ? 7672
Branches ? 0
=======================================
Hits ? 7305
Misses ? 367
Partials ? 0 ☔ View full report in Codecov by Sentry. |
579c859
to
495f912
Compare
Any feedback on this? Btw as commented in another PR, the AppVeyor failure looks unrelated. |
It's been about a year since last update on this merge. Is this repo abandoned somehow? What's the substitute library guys plz? |
joblib.memory is great but it is painful that it doesn't work quite right with async. |
I might be interested in using |
You can try using |
I wish this was part of the joblib's package ! |
I'm not using joblib for some years now but I could try rebasing this PR if there's interest in merging it, otherwise just lmk and I'll close it. |
I think this could be a nice addition, if you are up to rebasing the PR, i can take a look to help merge it. |
495f912
to
7db3218
Compare
7db3218
to
90a75ac
Compare
@tomMoral i rebased it, please take a look. Not sure what's the issue with the pypy timeout, the related tests are passing. |
Thanks a lot for rebasing and sorry for the delay in answering, I will take a look at this PR. I could not look at the pypy timeout but most probable is that it is due to a random failure of the pipeline on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks quite good!
I did a change on the name path
-> call_id
, did not expect this change to be in so many places 😅
Some cosmetic changes but overall I think this PR is very nice, thanks for the refactoring which makes the code much easier to read.
@tomMoral thanks for the review! I noticed in some places you rename path to cache_id and others to call_id; is this intentional? |
no, I think |
The failure is due to outdated install of sklearn's nightly build, which is fixed in #1567 |
Thanks a lot @gsakkis !! That's a nice feature! Sorry for the (very) slow process and thanks for keeping up with it 🙏 |
Better late than never, many thanks @tomMoral! |
Addresses #889, allowing coroutine functions (available in Python 3.5+) to be decorated with
joblib.memory
. In short, the decorated function's__call__
andcall_and_shelve
are coroutine functions that whenawait
ed return the output orMemorizedResult
, respectively, that would normally return if the original function was not a coroutine. A small subset of tests were ported fromtest_memory
to their asyncio equivalent to verify the expected behavior.In order to keep the asyncio-specific implementation as small as possible, I had to refactor a large portion of
memory.py
first. It's a separate commit so it can be reviewed and tested independently.