[MRG] FIX: issue with py27 when lz4 is not installed#740
[MRG] FIX: issue with py27 when lz4 is not installed#740ogrisel merged 11 commits intojoblib:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #740 +/- ##
==========================================
+ Coverage 95.23% 95.45% +0.21%
==========================================
Files 42 42
Lines 6128 6138 +10
==========================================
+ Hits 5836 5859 +23
+ Misses 292 279 -13
Continue to review full report at Codecov.
|
| elif cmethod == 'lz4' and with_lz4.args[0]: | ||
| # Skip the test if lz4 is not installed. We here use the with_lz4 | ||
| # skipif fixture whose argument is True when lz4 is not installed | ||
| raise SkipTest("lz4 is not installed.") |
There was a problem hiding this comment.
I am not sure this is right. With with_lz4, you will skip all compressors in _COMPRESSORS whereas we just want to skip cmethod=='lz4'.
Why do you need to change this?
There was a problem hiding this comment.
I was testing different things. But you are right and I reverted this change.
|
@tomMoral, I think this time the problem is correctly fixed. There was a problem in numpy_pickle.py. In the meantime, I added a test data compressed with lz4. This way the tests check that some data compressed with lz4 cannot be read if lz4 is not installed (and raise the right reason). Tell me if you think we should keep this and if you disagree, I can remove it. |
tomMoral
left a comment
There was a problem hiding this comment.
LGTM, with some small questions.
| # Run scikit-learn tests as integration / non-regression tests | ||
| - SKIP_TESTS="true" SKLEARN_TESTS="true" PYTHON_VERSION="3.6" NUMPY_VERSION="1.14" | ||
| - PYTHON_VERSION="2.7" NUMPY_VERSION="1.6" | ||
| - PYTHON_VERSION="2.7" NUMPY_VERSION="1.6" NO_LZ4="true" COVERAGE="true" |
There was a problem hiding this comment.
Why did you add coverage?
| # LZ4 compression is only supported and installation checked with | ||
| # python 3+. | ||
| if compress_method == 'lz4' and lz4 is None and PY3_OR_LATER: | ||
| raise ValueError(LZ4_NOT_INSTALLED_ERROR) |
There was a problem hiding this comment.
This feels weird to have different exception here. Can't we raise NotImplementedError?
There was a problem hiding this comment.
NotImplementedError would work for lzma in python2 because it's not part of the standard lib in this version of python.
Here we consider the user wants to dump using lz4 compression, without the package installed, so it's a value error. Python2 is skipped because the "not supported error" will be raised by the compressor wrapper when trying to write/read the data (see here).
There was a problem hiding this comment.
Thanks for the clarification.
All good to me then.
|
@lesteve, do you think we could merge this one ? |
* 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) ...
* 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) ...
When finished, this PR intends to fix #739.
For the moment, I'm trying to trigger the same issue in the CI.
The problem is that the lz4 package is always installed with python 2.7 in Travis. This hides the case raised in #739.
Note that lz4 is available in python2.7 but it doesn't work with file-like object the way they are used in joblib.