-
Notifications
You must be signed in to change notification settings - Fork 434
[MRG + 1] Fix skip test lz4 not installed #695
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.
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
[MRG + 1] Fix skip test lz4 not installed #695
Conversation
For old version of lz4, installed with conda, lz4.frame module does not have a LZ4FrameFile object. Ensure that this object is present to allow lz4 support.
Codecov Report
@@ Coverage Diff @@
## master #695 +/- ##
==========================================
- Coverage 95.12% 95.03% -0.09%
==========================================
Files 40 40
Lines 5663 5665 +2
==========================================
- Hits 5387 5384 -3
- Misses 276 281 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in a corner case of Travis CI build matrix: LZ4 is always installed when Numpy is installed and the test_joblib_compression_formats
is skipped when numpy is not installed but not when LZ4 is not installed and numpy is.
I'm fine with your changes but the line under the added elif
in test_numpy_pickle is not covered by the test.
It can be fixed by changing the Travis CI build matrix:
Maybe skip installation of LZ4 when Numpy is installed and Python version is 3.5.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not covered by the CI now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it should be covered with my last
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @aabadie shall we merge?
The coverage diff highlights lines that cannot be made covered in a deterministic manner and are unrelated to LZ4. |
And the diff coverage is low because the build without |
@@ -69,19 +69,23 @@ create_new_conda_env() { | |||
|
|||
create_new_conda_env | |||
|
|||
# Install py.test timeout to fasten failure in deadlocking tests | |||
PIP_INSTALL_PACKAGES="pytest-timeout" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit unrelated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just refactored all the pip install in one so I had to put this one in the list.
I think it makes think easier to read but I can put it directly in the pip install
if you prefer.
Maybe skipping lz4 with python 3.5 was not a good idea. I'm fine with an extra line in the matrix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM + 1. Let's merge this one.
* tag '0.12': (116 commits) Release 0.12 typo typo typo ENH add initializer limiting n_threads for C-libs (joblib#701) DOC better parallel docstring (joblib#704) [MRG] Nested parallel call thread bomb mitigation (joblib#700) MTN vendor loky2.1.3 (joblib#699) Make it possible to configure the reusable executor workers timeout (joblib#698) MAINT increase timeouts to make test more robust on travis DOC: use the .joblib extension instead of .pkl (joblib#697) [MRG] Fix exception handling in nested parallel calls (joblib#696) Fix skip test lz4 not installed (joblib#695) [MRG] numpy_pickle: several enhancements (joblib#626) Introduce Parallel.__call__ backend callbacks (joblib#689) Add distributed on readthedocs (joblib#686) Support registration of external backends (joblib#655) [MRG] Add a dask.distributed example (joblib#613) ENH use cloudpickle to pickle interactively defined callable (joblib#677) CI freeze the version of sklearn0.19.1 and scipy1.0.1 (joblib#685) ...
* releases: (121 commits) Release 0.12.1 fix kwonlydefaults key error in filter_args (joblib#715) MNT fix some "undefined name" flake8 warnings (joblib#713). from importlib import reload for Python 3 (joblib#675) MTN vendor loky2.1.4 (joblib#708) Release 0.12 typo typo typo ENH add initializer limiting n_threads for C-libs (joblib#701) DOC better parallel docstring (joblib#704) [MRG] Nested parallel call thread bomb mitigation (joblib#700) MTN vendor loky2.1.3 (joblib#699) Make it possible to configure the reusable executor workers timeout (joblib#698) MAINT increase timeouts to make test more robust on travis DOC: use the .joblib extension instead of .pkl (joblib#697) [MRG] Fix exception handling in nested parallel calls (joblib#696) Fix skip test lz4 not installed (joblib#695) [MRG] numpy_pickle: several enhancements (joblib#626) Introduce Parallel.__call__ backend callbacks (joblib#689) ...
No description provided.