Skip to content

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Aug 29, 2018

Fix #764

A non regression test is also provided.

With this PR, the snippets provided in #764 not returns the following:

In [1]: from joblib import Memory
In [2]: mem = Memory('/tmp/test')
In [3]: mem.store_backend
Out[1]: FileSystemStoreBackend(location="/tmp/test/joblib")
In [2]: from joblib._store_backends import FileSystemStoreBackend
   ...: backend = FileSystemStoreBackend()
   ...: backend
   ...: 
Out[2]: FileSystemStoreBackend(location="None")

@codecov
Copy link

codecov bot commented Aug 29, 2018

Codecov Report

Merging #765 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
- Coverage   95.42%   95.35%   -0.07%     
==========================================
  Files          44       44              
  Lines        6224     6245      +21     
==========================================
+ Hits         5939     5955      +16     
- Misses        285      290       +5
Impacted Files Coverage Δ
joblib/_store_backends.py 90.52% <100%> (+0.57%) ⬆️
joblib/test/test_memory.py 98.14% <100%> (+0.06%) ⬆️
joblib/memory.py 95.53% <100%> (ø) ⬆️
joblib/disk.py 81.66% <0%> (-6.67%) ⬇️
joblib/test/test_store_backends.py 91.17% <0%> (-5.89%) ⬇️

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 f64e1b8...2afc606. Read the comment docs.

@lesteve
Copy link
Member

lesteve commented Aug 29, 2018

Can you check that self.store_backend is not used in other classes __repr__. I seem to remember it is.

@aabadie
Copy link
Contributor Author

aabadie commented Aug 30, 2018

Can you check that self.store_backend is not used in other classes repr. I seem to remember it is.

I just did and you are right it was used in MemorizedResult and MemorizedFunc classes. It's fixed now.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM, just a small comment to simplify a test.

def test_filesystem_store_backend_repr(tmpdir):
# Verify string representation of a filesystem store backend.

repr_pattern = '{class_name}(location="{location}")'
Copy link
Contributor

Choose a reason for hiding this comment

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

class_name is always FileSystemStoreBackend in this test. Please just write:

repr_pattern = 'FileSystemStoreBackend(location="{location}")'

and then simplify the test accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! I just pushed the fix.

@aabadie
Copy link
Contributor Author

aabadie commented Aug 30, 2018

@ogrisel, is it ok to merge ?

memory = Memory(location=tmpdir.strpath, verbose=0)
memorized_func = memory.cache(my_func)

memorized_func_repr = '{class_name}(func={func}, location={location})'
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually class_name is always "MemorizedFunc" for memorized_func_repr and always "MemorizedResult" for memorized_result_repr so this test can also be simplified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed... Also done with Memory.

@ogrisel ogrisel merged commit 2f64d4c into joblib:master Aug 30, 2018
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Sep 4, 2018
* tag '0.12.3': (23 commits)
  Release 0.12.3
  Loky 2.2.1 (joblib#760)
  FIX: FileSystemStoreBackend string representation only returning location (path) (joblib#765)
  Add optional dependency on psutil
  MAINT remove brittle time based assertion in test (joblib#761)
  Fix a bug in nesting level computation with FallbackToBackend(SequentialBackend()) (joblib#759)
  Make docstring more consistent with project style
  Improved performance of call_and_shelve (joblib#757)
  Better test name
  Fix default context handling (joblib#754)
  cloudpickle 0.5.5 (joblib#756)
  Fixed error where filter args was consuming kwargs (joblib#750)
  FIX pickle roundtrip for Memory and related classes. (joblib#746)
  test that passes but timeout appveyor because of unclosed semaphore tracker (joblib#676)
  DOC: fine tune compressor example dataset size for ReadTheDocs (joblib#753)
  FIX: MemorizedResult not picklable (joblib#752)
  [MRG] Better message with py27 when lz4 is not installed (joblib#740)
  MNT remove mutable default value for backend_options parameter (joblib#748)
  MNT create test file for _store_backends module (joblib#749)
  DOC consistently use memory rather than mem in memory.rst (joblib#744)
  ...
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Sep 4, 2018
* releases: (23 commits)
  Release 0.12.3
  Loky 2.2.1 (joblib#760)
  FIX: FileSystemStoreBackend string representation only returning location (path) (joblib#765)
  Add optional dependency on psutil
  MAINT remove brittle time based assertion in test (joblib#761)
  Fix a bug in nesting level computation with FallbackToBackend(SequentialBackend()) (joblib#759)
  Make docstring more consistent with project style
  Improved performance of call_and_shelve (joblib#757)
  Better test name
  Fix default context handling (joblib#754)
  cloudpickle 0.5.5 (joblib#756)
  Fixed error where filter args was consuming kwargs (joblib#750)
  FIX pickle roundtrip for Memory and related classes. (joblib#746)
  test that passes but timeout appveyor because of unclosed semaphore tracker (joblib#676)
  DOC: fine tune compressor example dataset size for ReadTheDocs (joblib#753)
  FIX: MemorizedResult not picklable (joblib#752)
  [MRG] Better message with py27 when lz4 is not installed (joblib#740)
  MNT remove mutable default value for backend_options parameter (joblib#748)
  MNT create test file for _store_backends module (joblib#749)
  DOC consistently use memory rather than mem in memory.rst (joblib#744)
  ...
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