Replaced shutil and tempfile packages with joblib in caching transformers by kirilldolmatov · Pull Request #14113 · scikit-learn/scikit-learn

kirilldolmatov · 2019-06-18T12:57:10Z

Replaced shutil and tempfile packages with joblib functions according to Memory usage in official documentation

What does this implement/fix? Explain your changes.

This PR simplifies using of caching transformers within a Pipeline.

Any other comments?

This is my first PR.

Replaced shutil and tempfile packages with joblib functions according to Memory usage in [official documentation](https://joblib.readthedocs.io/en/latest/auto_examples/memory_basic_usage.html)

rth

Looks good except for one comment below. Thanks @kirilldolmatov!

rth · 2019-06-18T13:01:25Z

examples/compose/plot_compare_reduction.py

 # Create a temporary folder to store the transformers of the pipeline
-cachedir = mkdtemp()
-memory = Memory(location=cachedir, verbose=10)
+location = './cachedir'


Just "cachedir" for an OS independent path, otherwise LGTM. Thanks!

rth · 2019-06-18T13:18:41Z

examples/compose/plot_compare_reduction.py


 # Delete the temporary cache before exiting
-rmtree(cachedir)
+memory.clear(warn=False)


Have you checked that this also removes the location folder?

yes, I've checked after calling memory.clear(warn=False) location folder is removed. However, not main directory - '/cachedir', but subdirectory - 'sklearn' (./cachedir/joblib/sklearn). So after removing all caching files of transformers will be removed, but the directory (./cachedir/joblib) will remain.

OK, then we still want to remove the cachedir though.

I found smth that can meet our requirements: clear_path. But, it requires one extra code line.

1. Made location path OS independent 2. Add `clear_location()` for full removing location folder

thomasjpfan · 2019-06-19T14:51:43Z

This PR adds a 'cachedir' into the examples directory by directly following the joblib doc and then removes 'cachedir'. What was the issue with using a temporary folder (from mkdtemp) for caching in the example?

kirilldolmatov · 2019-06-20T07:40:29Z

The issue will be the same: it's also possible to use a temporary folder (from mkdtemp):

cachedir = mkdtemp()
memory = Memory(location=cachedir, verbose=10)
memory.clear(warn=False)
memory.store_backend.clear_location(cachedir)

thomasjpfan · 2019-06-21T20:45:09Z

Does

memory.clear(warn=False)
memory.store_backend.clear_location(cachedir)

do something that rmtree(cachedir) fails to do?

glemaitre · 2019-06-21T21:24:17Z

@thomasjpfan Both solutions are equivalent. We either use the standard lib or joblib to achieve the same result. I am +0.5 for the change (it is nice that people find out about the joblib.Memory feature).

kirilldolmatov · 2019-06-22T07:08:12Z

My initial PR replaced shutil and tempfile libs with joblib. Such solution looks simplifier, because we avoid import two extra libs and use only joblib. But, there is side effect - directory will remain after removing cache files. Therefore, I add extra one code line, that removes this directory. Moreover, such solution allows use mkdtemp, if user doesn't like to create folder in directly way.

glemaitre · 2019-06-25T15:35:29Z

examples/compose/plot_compare_reduction.py

 # Delete the temporary cache before exiting
-rmtree(cachedir)
+memory.clear(warn=False)
+memory.store_backend.clear_location(location)


IMO this is not a good idea. store_backend is not documented publicly. I would use the rmtree to remove the folder. In addition, I would add a comment that memory.clear() remove the cache used for the computation and rmtree() will remove the folder where the cache was localized.

I'd rather agree with you. I add clear_location(location) (in my second commit) for removing location directory after discussions above. IMHO, it isn't necessary to remove the folder, because it can be use in the future. If we're willing to accept this, than my first commit is actual. In another case, it might be better to use rmtree. My original wish was to replace three libs with one.

IMHO, it isn't necessary to remove the folder, because it can be use in the future

In general, I would agree. However, we don't want to have a remaining folder after the example execution. That's why, I would make a comment before the rmtree explaining why there is this call and that in practice, you probably don't need it.

My original wish was to replace three libs with one.

It does not matter since they come in the standard library of Python

OK, I've made new commit using rmtree(location) instead of clear_location(location)

glemaitre

If you could just add a small comment, something like: we remove the temporary directory earlier created (optional).

Otherwise LGTM

kirilldolmatov · 2019-06-26T11:33:33Z

examples/compose/plot_compare_reduction.py

 # Delete the temporary cache before exiting
 memory.clear(warn=False)
-memory.store_backend.clear_location(location)
+rmtree(location)


remove the temporary directory earlier created (optional)

rth · 2019-07-02T08:04:34Z

Merging, thanks!

…14113)

replaced shutil and tempfile packages with joblib

6c14eb7

Replaced shutil and tempfile packages with joblib functions according to Memory usage in [official documentation](https://joblib.readthedocs.io/en/latest/auto_examples/memory_basic_usage.html)

kirilldolmatov changed the title ~~replaced shutil and tempfile packages with joblib~~ Replaced shutil and tempfile packages with joblib in caching transformers Jun 18, 2019

rth reviewed Jun 18, 2019

View reviewed changes

update request according to comments

b5589ac

1. Made location path OS independent 2. Add `clear_location()` for full removing location folder

glemaitre self-requested a review June 25, 2019 15:28

glemaitre reviewed Jun 25, 2019

View reviewed changes

use rmtree for removing folder

c5b8c94

glemaitre approved these changes Jun 26, 2019

View reviewed changes

kirilldolmatov commented Jun 26, 2019

View reviewed changes

rth merged commit faa9406 into scikit-learn:master Jul 2, 2019

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

EXA Better joblib caching in plot_compare_reduction.py (scikit-learn#…

26b4d98

…14113)

Uh oh!

Conversation

kirilldolmatov commented Jun 18, 2019

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirilldolmatov Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kirilldolmatov commented Jun 20, 2019

Uh oh!

thomasjpfan commented Jun 21, 2019

Uh oh!

glemaitre commented Jun 21, 2019

Uh oh!

kirilldolmatov commented Jun 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirilldolmatov Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirilldolmatov Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kirilldolmatov Jun 18, 2019 •

edited

Loading

thomasjpfan commented Jun 19, 2019 •

edited

Loading

kirilldolmatov commented Jun 22, 2019 •

edited

Loading

kirilldolmatov Jun 25, 2019 •

edited

Loading

kirilldolmatov Jun 26, 2019 •

edited

Loading