equinor / seismic-zfp

Compress and decompress seismic data
GNU Lesser General Public License v3.0
67 stars 15 forks source link

pip install fails on ubuntu 18.04 #28

Closed fmell closed 4 years ago

fmell commented 4 years ago

Installing seismic-zfp with pip on ubuntu 18.04 fails due to the zgy2sgz dependency:

$ pip3 install seismic-zfp

...

Collecting zgy2sgz (from seismic-zfp)
  Could not find a version that satisfies the requirement zgy2sgz (from seismic-zfp) (from versions: )
No matching distribution found for zgy2sgz (from seismic-zfp)

This dependency is not needed for segy and sgz files. Could this be changed to an optional dependency?

da-wad commented 4 years ago

yes, this should definitely be an optional dependency!

In the ZgyConverter class' init you could do something like:

try:
    import zgy2sgz
    ... continue with the initialization
except ImportError:
    raise NotImplementedError("You can't use this wihtout the zgy2sgz package)

Not 100% sure how to handle this with requirements.txt / seutp.py though!

Can you add what version of pip you're using here? (default Ubuntu version or an upgrade?)

fmell commented 4 years ago

This is with the default pip version installed by apt (sudo apt install python3-pip) python version: 3.6.9 pip version: 9.0.1

I tried upgrading pip with pip to 20.2.1. This works, but I think it is not recommended to do this.

fmell commented 4 years ago

A possible way to solve this would be to add the zgy2sgz as a "extra" in the setup.py.

In the converter.py we could have this import:

try:
    import zgy2sgz
except ImportError:
    _has_zgy2sgz = False
else:
    _has_zgy2sgz = True

In the ZgyConverter class init we could do this

def __init__(self, file, filetype_checking=True, preload=False, chunk_cache_size=None):
    if not _has_zgy2sgz:
        raise ImportError("zgy2sgz is required for SgzConverter. Install optional dependency seismic-zfp[zgy] with pip.")
    super().__init__(file, filetype_checking, preload, chunk_cache_size)

And in the setup.py we add this:

extras_require={
    'zgy': ['zgy2sgz']
},

This would let the user install seismic-zfpas normal with pip, but without the zgy2sgz dependency. To install seismic-zfp with zgy support you would install seismic-zfp[zgy].

See #29 for an example.

da-wad commented 4 years ago

Yes, I think this is the right way to go about solving the issue. Will continue the discussion on #29 .