[MRG] Custom store backends API #397

aabadie · 2016-09-21T16:05:39Z

~~This is a WIP PR just to check how this refactoring behaves on Windows~~. I'm also interested in API comments.
Normally tests should pass (at least on Linux/Python 3.5).
I tried to keep as much as possible the behavior of the test suite so normally it shouldn't introduce regression.

Here's a list of remaining things:

Backport new cache reduction function in the store API (it works like the store is a file system behind). Comment/proposal of API welcome here.
Provide a gist with an example working with S3 (I still need to update the one I have with my recent changes in the API)
More testing
Documentation

aabadie · 2016-09-22T14:22:38Z

For the record, I created a new repository which aims at implementing extra store backends for Joblib. This is at a very early stage and only a backend for S3 is provided.

https://github.com/aabadie/joblibstore

aabadie · 2016-09-24T21:59:08Z

I refactored my initial proposal of backend API to something more simple. Now a store backend inherit from a backend base class and store manager mixin. The latter containing all the complex logic and the former, only a few set of required functions. Adding a new backend is now simpler.

Apart from that, I backported the cache reduction mecanism to this refactoring and could also make the tests pass again without too much changes.

aabadie · 2016-09-27T12:27:34Z

I reworked slightly the initial way of registering custom backend because I had problems with hdfs. Now the strategy is to be explicit (rather than implicit ;) ) by using a backend name associated with a location when instanciating the memory object. Example:

from joblib import Memory
from joblib import register_store_backend

# register a custom store backend
register_store_backend('mybackend', StoreBackendSubclass)

# cache myfunc using Memory
mem = Memory(location='joblib/cache', backend='mybakend', compress=True, mybackend_option='option')
myfunc = mem.cache(myfunc)
[...]

by default, the backend parameter is set to local which corresponds to the regular (historical) file system backend.

I changed the status of this PR as [MRG] because I think it's a pretty good shape now. The joblibstore project also gives examples based on S3 and HDFS.

lesteve · 2016-11-23T13:01:35Z

There was some talk at one point, that we needed an additional level for the cache folder in order for the cloud storage backend to have a chance of being functional. @aabadie have you looked at it?

IIRC folder layout would look like this:

full/module/name/function_name/function_code_hash/arguments_hash/output.pkl

Where function_code_hash is the additional level. Also this would very likely fix the race condition highlighted at #111.

aabadie · 2016-11-24T15:41:06Z

There was some talk at one point, that we needed an additional level for the cache folder in order for the cloud storage backend to have a chance of being functional. @aabadie have you looked at it?

Not yet. Will do asap.

IIRC folder layout would look like this: ...

agreed, do you prefer to have this done in this PR or in another one ? (+1 for the first one)

lesteve · 2016-11-24T16:34:28Z

agreed, do you prefer to have this done in this PR or in another one ? (+1 for the first one)

different PR please! It is a small and incremental change and would fix the race condition mentioned above, makes it way easier to review.

aabadie · 2017-01-05T09:42:27Z

There was some talk at one point, that we needed an additional level for the cache folder in order for the cloud storage backend to have a chance of being functional. @aabadie have you looked at it?

I started to look at this and now have something nearly ready. I just have some comments:

before opening a PR, I'd like to have [MRG] Refactor test_memory.py as per pytest design. #466 merged first to avoid large conflicts there (which is also already the case here in [MRG] Custom store backends API #397)
similar this PR ([MRG] Custom store backends API #397), this extra subdirectory change will have a lot impact in term of code change and with the cache behavior : it solves the problems of collision in cache with functions with the same name but different internal code. I would prefer to have this one merged ([MRG] Custom store backends API #397) first before going further.
last but not least, as discussed IRL with @GaelVaroquaux, what should be the cache invalidation in this case ? if someone changes a function code, should all the cache of the same function name be invalidated ? do we want to keep it and use LRU replacement policy ?

lesteve · 2017-01-10T10:10:44Z

@aabadie #466 has been merged

lesteve · 2017-01-10T10:54:39Z

Just curious, there was some idea of being able to have a custom store for joblib.load/dump but actually at the moment this amounts to have a file-like object for your store (as I am guessing hdfs3.HDFileSystem().open does for example)?

aabadie · 2017-01-10T15:41:40Z

Just curious, there was some idea of being able to have a custom store for joblib.load/dump but actually at the moment this amounts to have a file-like object for your store (as I am guessing hdfs3.HDFileSystem().open does for example)?

Not sure I understand correctly but we have the same requirement with the actual refactoring. Any store backend should have a file-like object in order to be able to dump/load in it.

aabadie · 2017-01-10T15:42:10Z

@aabadie #466 has been merged

Thanks @lesteve, I'll fix the conflicts here asap.

aabadie · 2017-01-10T18:16:27Z

@lesteve, rebased with current master

aabadie · 2017-01-11T08:11:22Z

joblib/test/test_memory.py

+        "Does nothing"
+
+
+def test_register_invalid_store_backends():


I think this one is a good candidate for pytest @parametrize and split in 2 separate test functions.

aabadie · 2017-01-13T09:15:32Z

ping @lesteve @ogrisel

aabadie · 2017-01-17T18:17:47Z

While testing joblibstore with the hdfs3 backend, I get serious issues : hdfs3 file object doesn't work with joblib, and even more with pickle itself. I opened 2 issues regarding the problem: dask/hdfs3#107 and dask/hdfs3#108.

Other remarks:

With the S3 backend using s3fs, it's not possible to get the last_accessed metadata of a file. We'll have to find another solution in this case, maybe the metadata.json file
The HDFS3 filesystem object provides this information otherwise

eyadsibai · 2017-02-24T13:37:33Z

I am looking forward for this to be merged. Is there anything to do to speed it up? I would love to have s3 or google storage as backend stores to joblib

aabadie · 2017-02-27T07:46:14Z

Is there anything to do to speed it up?

Yes : reviewing the changes of this PR and test. BTW, joblibstore is still alive now better tested. It also fully works (at least for me) with S3. With HDFS, there are still issues with numpy arrays but that doesn't come from joblib or joblibstore : this is an issue in the hdfs3 package.

aabadie · 2017-03-01T20:45:32Z

@eyadsibai, it would be really great if you could give a try to the joblib store backends implemented in joblibstore (try S3 first) and report bugs if you find any
Let us know if you have problems setting things up.

eyadsibai · 2017-03-01T22:26:31Z

@aabadie sure will do

codecov · 2017-04-19T15:43:57Z

Codecov Report

Merging #397 into master will decrease coverage by 0.16%.
The diff coverage is 93.29%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
- Coverage   94.98%   94.81%   -0.17%     
==========================================
  Files          38       39       +1     
  Lines        5005     5193     +188     
==========================================
+ Hits         4754     4924     +170     
- Misses        251      269      +18

Impacted Files	Coverage Δ
joblib/disk.py	`81.66% <0%> (ø)`	⬆️
joblib/__init__.py	`100% <100%> (ø)`	⬆️
joblib/test/test_disk.py	`100% <100%> (ø)`	⬆️
joblib/_store_backends.py	`91% <91%> (ø)`
joblib/test/test_memory.py	`97.76% <95.03%> (-1.6%)`	⬇️
joblib/memory.py	`94.95% <95.08%> (+1.55%)`	⬆️
joblib/_parallel_backends.py	`94.73% <0%> (-0.44%)`	⬇️
joblib/test/test_parallel.py	`96.1% <0%> (-0.41%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba06b7e...ce0f664. Read the comment docs.

GaelVaroquaux · 2018-01-22T10:40:45Z

I have some cosmetic changes to do on this PR. @aabadie : is it OK if I push them to your branch? I hope that you don't have changes not pushed here.

GaelVaroquaux · 2018-01-22T11:28:54Z

I did a thorough read of this PR, and I don't see any major things to change. I do see two minor things that I'd like to change:

There are many places where "kwargs" are used. I'd like to decrease these, as that's hard to read because it's too generic. I see two paths (and will probably end up using a combination of both):
- I might simply turn "**kwargs" to "store_args" and document that "store_args" is a dictionary of store-specific arguments.
- Some arguments can be taken out of the kwargs and simply not used by some stores (but maybe none are eligible here)
The path in the store is specified as "func_id", "args_id". As we are considering in Expose storing dumping of objects to disk #593 a more general usage of the store backends, I'd like to change it to be more general.
- How about taking as inputs "path_elements", which would be a list of strings? Memory would need to be changed to generate it, but it seems easy.

GaelVaroquaux · 2018-01-22T15:46:05Z

While working on the code I also noted that the stores use the vocabulary "result" a lot. We should probably change that to "object", as the store will be used for other things than objects.

GaelVaroquaux · 2018-01-22T21:33:34Z

I've started changing the names of the API so that the terminology of the store is less specialized to caching. @aabadie : I am sure that this will break other backends. My apologies. But now is better than later.

GaelVaroquaux · 2018-01-22T21:53:21Z

@aabadie : we should have a discussion on how we evolve this object in the context of #593. I think that we can easily make naming a bit more universal. Let's talk on Wednesday.

aabadie · 2018-01-23T07:27:03Z

I am sure that this will break other backends

No problem, this was expected. We'll just have to give them a bit of love after we stabilize the API here (and eventually merge this PR).

aabadie · 2018-01-23T08:05:34Z

Another thing that is not addressed by this PR is some documentation about the new behaviour.

GaelVaroquaux · 2018-01-23T08:27:53Z

Another thing that is not addressed by this PR is some documentation about the new behaviour.

Yes :)

aabadie · 2018-01-24T10:16:00Z

joblib/_store_backends.py

+
+    The StoreBackend subclass has to implement 3 methods: create_location,
+    clear_location and configure. The StoreBackend also has to provide
+    open_object and object_exists methods by monkey matching them


s/matching/patching/

aabadie · 2018-01-24T16:37:59Z

@GaelVaroquaux, I could fix your CI issues and renamed Memory/MemorizedFunc store to store_backend. CI are passing.

I wanted to rebase to latest master but had some commits conflicting with this branch, so I squashed all commits in this PR in a single one, rebased and forced the push. We lost your/my recent history of changes. Hope this is fine.

- make internal methods of store backend base class private - document backend base class methods

GaelVaroquaux · 2018-01-25T16:28:20Z

Merging. Congratulations!

lesteve · 2018-01-29T14:16:50Z

Very nice!

IIRC for cloud backends, updating an object at a given location is not possible, so you need to delete the object and create a new one at the same location but there is no guarantee which version (old or new) you will get.

The fix discussed a while ago involved adding an additional level of folder (one per function hash) in order to avoid sharing the cache between different versions of the cached function. See my old comment.

A few issues to solve:

what kind of backward-compatibility do we want with the old cache layout? I'd say we want at least to still be able to read it with some warning
is it doable to move the old cache argument-hash folder into the func-code-hash folder so that the migration to the new folder layout is transparent for the user.

cc @ogrisel since he was the one that originally mentionned this issue about cloud backends.

lesteve · 2018-02-27T16:18:13Z

joblib/memory.py

+        if cachedir is not None:
+            if location is None:
+                warnings.warn("cachedir option is deprecated since version "
+                              "0.10 and will be removed after version 0.12.\n"


I am not sure what the versions should be but at the moment they are inconcistent in this PR. This one says 0.10 -> 0.12. Other places say 0.11 -> 0.13.

I am guessing it would make sense to release a 0.12 with all the changes that are going into master at the moment.

- Looks like an oversight introduced in joblib#397

- Looks like an oversight introduced in #397

aabadie added the in progress label Sep 21, 2016

aabadie force-pushed the storage_backend_2 branch from 6c7dbf1 to 02b82fd Compare September 23, 2016 13:49

aabadie changed the title ~~[WIP] Custom store backend~~ [MRG] Custom store backends API Sep 27, 2016

aabadie added need Review and removed in progress labels Oct 3, 2016

GaelVaroquaux mentioned this pull request Oct 5, 2016

Nested Parallelism scikit-learn/scikit-learn#3754

Closed

aabadie force-pushed the storage_backend_2 branch 2 times, most recently from 57261ac to 2020389 Compare January 10, 2017 18:12

aabadie commented Jan 11, 2017

View reviewed changes

aabadie force-pushed the storage_backend_2 branch from c4407bc to c2d9800 Compare January 11, 2017 14:14

aabadie force-pushed the storage_backend_2 branch from bc9ce1a to 164cc0d Compare April 19, 2017 14:59

aabadie commented Jan 24, 2018

View reviewed changes

aabadie force-pushed the storage_backend_2 branch from 21f50bd to 9a257b1 Compare January 24, 2018 14:53

add storage backend interface + filesystem storage backend

375e216

aabadie force-pushed the storage_backend_2 branch from 14f0e46 to 375e216 Compare January 24, 2018 16:35

aabadie added 3 commits January 24, 2018 22:18

rework some docstring

d411e47

let memory manage the base location

60573f4

polishing:

ce0f664

- make internal methods of store backend base class private - document backend base class methods

GaelVaroquaux merged commit 89743fa into joblib:master Jan 25, 2018

aabadie mentioned this pull request Jan 26, 2018

adapt to new Memory api joblib/joblib-hadoop#8

Merged

aabadie deleted the storage_backend_2 branch February 27, 2018 15:43

lesteve mentioned this pull request Feb 27, 2018

Memory object does not have a cachedir attribute in master #638

Closed

lesteve reviewed Feb 27, 2018

View reviewed changes

lesteve mentioned this pull request Apr 26, 2018

MISC: fix typo in private function name #669

Merged

jdanbrown added a commit to jdanbrown/joblib that referenced this pull request May 16, 2018

memory: Fix %{0} -> {0} in str.format

0075abb

- Looks like an oversight introduced in joblib#397

jdanbrown mentioned this pull request May 16, 2018

memory: Fix %{0} -> {0} in str.format #678

Merged

ogrisel pushed a commit that referenced this pull request May 22, 2018

memory: Fix %{0} -> {0} in str.format (#678)

8c79fd8

- Looks like an oversight introduced in #397

lesteve mentioned this pull request Jul 6, 2018

v0.12: 'MemorizedResult' object has no attribute 'argument_hash' #714

Closed

This was referenced Aug 20, 2018

pickling breaks between joblib version 0.11 and 0.12.2 #747

Closed

Fixed error where filter args was consuming kwargs #750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Custom store backends API #397

[MRG] Custom store backends API #397

aabadie commented Sep 21, 2016 •

edited

Loading

aabadie commented Sep 22, 2016 •

edited

Loading

aabadie commented Sep 24, 2016

aabadie commented Sep 27, 2016

lesteve commented Nov 23, 2016

aabadie commented Nov 24, 2016

lesteve commented Nov 24, 2016

aabadie commented Jan 5, 2017

lesteve commented Jan 10, 2017

lesteve commented Jan 10, 2017 •

edited

Loading

aabadie commented Jan 10, 2017

aabadie commented Jan 10, 2017

aabadie commented Jan 10, 2017

aabadie Jan 11, 2017

aabadie commented Jan 13, 2017

aabadie commented Jan 17, 2017

eyadsibai commented Feb 24, 2017

aabadie commented Feb 27, 2017

aabadie commented Mar 1, 2017

eyadsibai commented Mar 1, 2017 •

edited

Loading

codecov bot commented Apr 19, 2017 •

edited

Loading

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

aabadie commented Jan 23, 2018

aabadie commented Jan 23, 2018

GaelVaroquaux commented Jan 23, 2018 via email

aabadie Jan 24, 2018

aabadie commented Jan 24, 2018 •

edited

Loading

GaelVaroquaux commented Jan 25, 2018

lesteve commented Jan 29, 2018

lesteve Feb 27, 2018

[MRG] Custom store backends API #397

[MRG] Custom store backends API #397

Conversation

aabadie commented Sep 21, 2016 • edited Loading

aabadie commented Sep 22, 2016 • edited Loading

aabadie commented Sep 24, 2016

aabadie commented Sep 27, 2016

lesteve commented Nov 23, 2016

aabadie commented Nov 24, 2016

lesteve commented Nov 24, 2016

aabadie commented Jan 5, 2017

lesteve commented Jan 10, 2017

lesteve commented Jan 10, 2017 • edited Loading

aabadie commented Jan 10, 2017

aabadie commented Jan 10, 2017

aabadie commented Jan 10, 2017

aabadie Jan 11, 2017

Choose a reason for hiding this comment

aabadie commented Jan 13, 2017

aabadie commented Jan 17, 2017

eyadsibai commented Feb 24, 2017

aabadie commented Feb 27, 2017

aabadie commented Mar 1, 2017

eyadsibai commented Mar 1, 2017 • edited Loading

codecov bot commented Apr 19, 2017 • edited Loading

Codecov Report

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

GaelVaroquaux commented Jan 22, 2018

aabadie commented Jan 23, 2018

aabadie commented Jan 23, 2018

GaelVaroquaux commented Jan 23, 2018 via email

aabadie Jan 24, 2018

Choose a reason for hiding this comment

aabadie commented Jan 24, 2018 • edited Loading

GaelVaroquaux commented Jan 25, 2018

lesteve commented Jan 29, 2018

lesteve Feb 27, 2018

Choose a reason for hiding this comment

aabadie commented Sep 21, 2016 •

edited

Loading

aabadie commented Sep 22, 2016 •

edited

Loading

lesteve commented Jan 10, 2017 •

edited

Loading

eyadsibai commented Mar 1, 2017 •

edited

Loading

codecov bot commented Apr 19, 2017 •

edited

Loading

aabadie commented Jan 24, 2018 •

edited

Loading