Skip to content

Conversation

tomMoral
Copy link
Contributor

No description provided.

tomMoral added 2 commits June 14, 2018 11:18
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
Copy link

codecov bot commented Jun 14, 2018

Codecov Report

Merging #695 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
joblib/test/test_numpy_pickle.py 97.82% <100%> (ø) ⬆️
joblib/compressor.py 93.2% <100%> (ø) ⬆️
joblib/disk.py 81.66% <0%> (-6.67%) ⬇️
joblib/backports.py 93.75% <0%> (-2.09%) ⬇️
joblib/_store_backends.py 90.47% <0%> (-0.53%) ⬇️
joblib/_parallel_backends.py 97% <0%> (+0.42%) ⬆️

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 da01924...230f464. Read the comment docs.

@tomMoral tomMoral requested a review from aabadie June 15, 2018 11:48
Copy link
Contributor

@aabadie aabadie left a 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.")
Copy link
Contributor

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.

Copy link
Contributor Author

@tomMoral tomMoral Jun 15, 2018

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

@ogrisel ogrisel self-requested a review June 15, 2018 14:49
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, @aabadie shall we merge?

@ogrisel
Copy link
Contributor

ogrisel commented Jun 15, 2018

The coverage diff highlights lines that cannot be made covered in a deterministic manner and are unrelated to LZ4.

@tomMoral
Copy link
Contributor Author

tomMoral commented Jun 15, 2018

And the diff coverage is low because the build without lz4 do not have coverage support.
I can add it in the matrix if you want.

@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit unrelated

Copy link
Contributor Author

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.

@aabadie
Copy link
Contributor

aabadie commented Jun 15, 2018

the diff coverage is low because the build without lz4 do not have coverage support.

Maybe skipping lz4 with python 3.5 was not a good idea. I'm fine with an extra line in the matrix.

Copy link
Contributor

@aabadie aabadie left a 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.

@aabadie aabadie changed the title Fix skip test lz4 not installed [MRG + 1] Fix skip test lz4 not installed Jun 18, 2018
@aabadie aabadie merged commit f3324ee into joblib:master Jun 18, 2018
@tomMoral tomMoral deleted the FIX_skip_test_lz4_not_installed branch June 20, 2018 07:56
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Jul 28, 2018
* 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)
  ...
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Jul 28, 2018
* 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)
  ...
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