search

kiyo-masui / bitshuffle

Filter for improving compression of typed binary data.
Other
215 stars 76 forks source link

Loading libhdf5 dynamically from bitshuffle.h5 #81

Closed t20100 closed 3 years ago

t20100 commented 4 years ago

This PR proposes to adapt the dynamic loading of libhdf5 used in https://github.com/silx-kit/hdf5plugin to bitshuffle.h5. This allows to build bitshuffle without linking to a specific version of the HDF5 library. This would enable the creation of wheels for bitshuffle.

This PR does not modify the plugin part of bitshuffle, only bitshuffle.h5. I only tested in on Linux (test_h5filter pass). If you are interested by this approach, please let me know.

I can test it on Windows as well, I can't however test it on macOS.

related to issue #76

nritsche commented 3 years ago

Thanks so much for this contribution @t20100. We should change this in the documentation, too and remove the h5py source-build from the requirements.

t20100 commented 3 years ago

Hi, If you are interested I can have a second look at this PR.

You should no longer need to build h5py` and share the same libhdf5 with the filter, but you still need the HDF5 headers to build the filter (unless you bundle them here or provide wheels).

BTW, we bundle the bitshuffle hdf5 filter along with others in hdfplugin.

nritsche commented 3 years ago

If you are interested I can have a second look at this PR.

Great! That would be amazing.

You should no longer need to build h5py` and share the same libhdf5 with the filter, but you still need the HDF5 headers to build the filter (unless you bundle them here or provide wheels).

BTW, we bundle the bitshuffle hdf5 filter along with others in hdfplugin.

Let's get this PR merged. I think what is mainly missing is

Once that's done and this PR merged, we can get a working version to pypi.

jrs65 commented 3 years ago

@james-s-willis maybe you can take a look at this? I think if we merge it before the zstd changes it may keep the FRB folks happier as we can maybe binary wheels and save them some of the bitshuffle build complexity (which we've just increased).

james-s-willis commented 3 years ago

I think this is ready to be reviewed, @jrs65.

james-s-willis commented 3 years ago

@jrs65 I tried this on our local machine magenta. I removed bitshuffle and h5py and reinstalled with pip3 install h5py. The unit tests still pass:

[willis@magenta tests]$ python3 -m pytest
=============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.7.9, pytest-5.3.0, py-1.8.0, pluggy-0.13.0
rootdir: /home/willis/dynamic-hdf5-loading
plugins: timeout-1.4.2, forked-1.1.3, xdist-1.30.0, localserver-0.5.0, cpp-1.1.0, pyfakefs-4.5.0
collected 61 items                                                                                                                                                                                                

test_ext.py ........................................................                                                                                                                                        [ 91%]
test_h5filter.py ...                                                                                                                                                                                        [ 96%]
test_h5plugin.py .                                                                                                                                                                                          [ 98%]
test_regression.py .                                                                                                                                                                                        [100%]

=============================================================================================== 61 passed in 1.89s ================================================================================================

Should I merge this now?

jrs65 commented 3 years ago

Should I merge this now?

Go for it!