search

chime-experiment / ch_util

CHIME utilities
https://chime-experiment.github.io/ch_util
MIT License
2 stars 3 forks source link

Optional requirement for bitshuffle #8

Closed danielemichilli closed 3 years ago

danielemichilli commented 3 years ago

Bitshuffle has problems with python >=3.7 and it is not needed by many funtionalities of ch_util. In fact, we do not use it in CHIME/FRB baseband analysis. I edited the setup to have bitshuffle as an extra requirement

jrs65 commented 3 years ago

@danielemichilli this is going to be a no to merging this one. Bitshuffle is essential for reading our data. It works fine on Python 3.7 upwards, but you need to build from the upstream git repo.

Note that we are looking at improving the installation process for bitshuffle, but haven't got around to it yet.

jrs65 commented 3 years ago

If you don't need it and have trouble building it you can probably just create a stub package that mocks it, and install that before ch_util. We are hopefully going to fix up bitshuffle itself soon too, but there isn't a timescale on it.

jrs65 commented 3 years ago

Actually using a stub might not work with the dependency link we've put in requirements. @anjakefala would that work or break?

danielemichilli commented 3 years ago

I am not sure I see where the problem of an optional requirement is, this is a solution suggested in PEP - the requirement is not removed, it is just optional. Your suggestion doesn't look like a clean workaround and requiring all the users to install a package not needed in most of the modules is not a good option to me. I suggest never having bitshuffle as a necessary requirement but, if really needed, adding it back once fixed

danielemichilli commented 3 years ago

And actually, I am just noticing that even chimedb_config is an optional module

danielemichilli commented 3 years ago

BTW how do you manage to install bitshuffle with python > 3.6? I am using 

HDF5_DIR="/usr/lib/x86_64-linux-gnu/hdf5/serial/" pip install --no-binary=h5py h5py==2.10.0
HDF5_DIR="/usr/lib/x86_64-linux-gnu/hdf5/serial/" pip install --no-binary=bitshuffle bitshuffle==0.3.5

And it just fails when compiling bitshuffle

jrs65 commented 3 years ago

Practically we would need to change the way we install our entire toolchain everywhere including all the CI builds to ensure we specify the optional requirements, this PR is just going to generate an annoying series of things we would need to fix in a bunch of other places.

However at broader level, while from an FRB perspective bitshuffle might be optional, from a CHIME perspective it's not: having an installation of ch_util essentially serves as a promise that you can read and interact with CHIME data.

chimedb_config is different. It's optional because it is a private repo containing our database credentials, and we don't want to be forced to be able to pull it for CI runs etc, but It is also truly optional in that you can create an rc file with the same credentials to access the db without installing that module.

As I said you need to install bitshuffle from the upstream git repo. I usually do something like:

export HDF5_DIR=<path>   # although not always necessary
pip install --no-binary=h5py h5py
pip install --no-binary=bitshuffle git+https://github.com/kiyo-masui/bitshuffle.git

In practice the change in pip's resolver is messing us up in many places at the moment, so while I think that'll work it may get a little confused. If it is you might need to add something like --use-deprecated=legacy-resolver.

jrs65 commented 3 years ago

(you also might need to add an #egg=bitshuffle to the end of the bitshuffle pip line)

anjakefala commented 3 years ago

Actually using a stub might not work with the dependency link we've put in requirements. @anjakefala would that work or break?

It will work with the old resolver, and not with the new one!

The old resolver will see that bitshuffle is already installed, and not attempt to update it. The new resolver will try to clone the repo and do a version comparison, where the "url of origin" is part of the version comparison.

danielemichilli commented 3 years ago

As I said you need to install bitshuffle from the upstream git repo. I usually do something like:

export HDF5_DIR=<path>   # although not always necessary
pip install --no-binary=h5py h5py
pip install --no-binary=bitshuffle git+https://github.com/kiyo-masui/bitshuffle.git

After trying many different combinations, this finally made the trick, thanks! (It works with python 3.9)