Skip to content

[MRG] FIX: issue with py27 when lz4 is not installed #740

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.

Sign up for GitHub

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

Merged
ogrisel merged 11 commits into from
Aug 22, 2018
Merged

[MRG] FIX: issue with py27 when lz4 is not installed #740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cef3abb
trigger the issue with py27
aabadie Aug 8, 2018
072c037
proposing a fix
aabadie Aug 9, 2018
4b71a08
activate coverage for both python2
aabadie Aug 9, 2018
e152cf3
improving lz4 testing
aabadie Aug 9, 2018
d33fb08
...
aabadie Aug 9, 2018
960ef7d
add test data with lz4 compression (just for python 3.6)
aabadie Aug 9, 2018
aabaece
reverting unintended change
aabadie Aug 9, 2018
7b10739
improving lz4 testing
aabadie Aug 9, 2018
acdbdb3
commenting
aabadie Aug 9, 2018
e800003
fix diff hit
aabadie Aug 9, 2018
f4e788f
fix diff hit 2
aabadie Aug 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ env:
matrix:
# 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"
Copy link
Copy Markdown
Contributor

@tomMoral tomMoral Aug 10, 2018

Choose a reason for hiding this comment

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

Why did you add coverage?

Copy link
Copy Markdown
Contributor Author

@aabadie aabadie Aug 10, 2018

Choose a reason for hiding this comment

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

To cover this line

Copy link
Copy Markdown
Contributor

@tomMoral tomMoral Aug 11, 2018

Choose a reason for hiding this comment

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

ok

- PYTHON_VERSION="2.7" NUMPY_VERSION="1.8" COVERAGE="true"
- PYTHON_VERSION="3.4" NUMPY_VERSION="1.10"
# NUMPY_VERSION not set means numpy is not installed
Expand Down
4 changes: 3 additions & 1 deletion joblib/numpy_pickle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,9 @@ def dump(value, filename, compress=0, protocol=None, cache_size=None):
else:
compress_level = compress

if compress_method == 'lz4' and lz4 is None:
# 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)
Copy link
Copy Markdown
Contributor

@tomMoral tomMoral Aug 10, 2018

Choose a reason for hiding this comment

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

This feels weird to have different exception here. Can't we raise NotImplementedError?

Copy link
Copy Markdown
Contributor Author

@aabadie aabadie Aug 10, 2018

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor

@tomMoral tomMoral Aug 11, 2018

Choose a reason for hiding this comment

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

Thanks for the clarification.
All good to me then.


if (compress_level is not None and
Expand Down
Binary file added joblib/test/data/joblib_0.12.2_pickle_py36_np114.pkl.lz4
View file
Open in desktop
Binary file not shown.
28 changes: 19 additions & 9 deletions joblib/test/test_numpy_pickle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,12 +389,17 @@ def _check_pickle(filename, expected_list):
Note: currently only pickles containing an iterable are supported
by this function.
"""
if (not PY3_OR_LATER and (filename.endswith('.xz') or
filename.endswith('.lzma') or
filename.endswith('.lz4'))):
# lzma and lz4 are not supported for python versions < 3.3
with raises(NotImplementedError):
numpy_pickle.load(filename)
if not PY3_OR_LATER:
if filename.endswith('.xz') or filename.endswith('.lzma'):
# lzma is not implemented in python versions < 3.3
with raises(NotImplementedError):
numpy_pickle.load(filename)
elif filename.endswith('.lz4'):
# lz4 is not supported for python versions < 3.3
with raises(ValueError) as excinfo:
numpy_pickle.load(filename)
assert excinfo.match("lz4 compression is only available with "
"python3+")
return

version_match = re.match(r'.+py(\d)(\d).+', filename)
Expand Down Expand Up @@ -439,6 +444,9 @@ def _check_pickle(filename, expected_list):
message = ('You may be trying to read with '
'python 3 a joblib pickle generated with python 2.')
assert message in str(exc)
elif filename.endswith('.lz4') and with_lz4.args[0]:
assert isinstance(exc, ValueError)
assert LZ4_NOT_INSTALLED_ERROR in str(exc)
else:
raise
else:
Expand All @@ -454,7 +462,6 @@ def _check_pickle(filename, expected_list):
assert message in str(e.args)


@with_lz4
@with_numpy
def test_joblib_pickle_across_python_versions():
# We need to be specific about dtypes in particular endianness
Expand Down Expand Up @@ -1055,10 +1062,13 @@ def test_lz4_compression_without_lz4(tmpdir):
# Check that lz4 cannot be used when dependency is not available.
fname = tmpdir.join('test.nolz4').strpath
data = 'test data'
msg = LZ4_NOT_INSTALLED_ERROR
if not PY3_OR_LATER:
msg = "lz4 compression is only available with python3+"
with raises(ValueError) as excinfo:
numpy_pickle.dump(data, fname, compress='lz4')
excinfo.match(LZ4_NOT_INSTALLED_ERROR)
excinfo.match(msg)

with raises(ValueError) as excinfo:
numpy_pickle.dump(data, fname + '.lz4')
excinfo.match(LZ4_NOT_INSTALLED_ERROR)
excinfo.match(msg)