medaka icon indicating copy to clipboard operation
medaka copied to clipboard

Fixes missing symbols for libdeflate.

Open ccoulombe opened this issue 2 years ago • 4 comments

Version 1.4.4 and 1.5 are affected.

Building htslib.a on a system that provide libdeflate ends up with an import error:

ImportError: virtual_envs/medaka_v1.4.4/lib/python3.8/site-packages/libmedaka.abi3.so: undefined symbol: libdeflate_crc32

A quick solution is to link against libdeflate. Alternative solutions :

  • Build htslib.so and link to it
  • Use system htslib when available.

ccoulombe avatar Dec 15 '21 19:12 ccoulombe

Hi @ccoulombe

Thanks for this report. I wasn't aware that the htslib build will automatically try to use libdeflate if its present, only that there was an option to enable it.

Out of interest, why do you want to compile medaka yourself rather than use either the PyPI or bioconda distributions?

cjw85 avatar Dec 15 '21 19:12 cjw85

We provide optimized wheels/software for our HPC systems. Manylinux wheels are generic wheels or say less-optimized that can run mostly everywhere. And Conda is to be avoided at all costs since it mainly creates issues on our systems.

When htslib is configured, it detects libdeflate and then use it.

Letting medaka uses a system htslib would be nice, as it is done for samtools or minimap2

ccoulombe avatar Dec 15 '21 20:12 ccoulombe

Before accepting this it would be preferable that the behaviour (libhts options) were more explicit. With all the automatic checks going on in the libhts autoconf build its perfectly possible to make a wheel on one system that will have linking issues on another. This could consist simply of a HTSLIB_CONF_FLAGS variable in the Makefile (the other extensions like curl, crypto, lzma etc should be explicit also), and corresponding flags passed to the cffi build.py

cjw85 avatar Jan 04 '22 10:01 cjw85

@ccoulombe I'm cleaning up medaka issues and PRs.

Our build process now compiles libdeflate, the only reason a system libdeflate wasn't being linked was because one wasn't available inside the wheel build containers (it was an error to not realise the performance impact). I think this probably renders this PR moot.

I'm interested to know how much more performant your custom builds are. Maybe there are things we can change in our builds for the benefit of all/most users.

cjw85 avatar Aug 12 '22 09:08 cjw85

@cjw85 just commenting that I would also like this feature. I'm building a package for medaka with the from-source package manager used in our HPC cluster (https://spack.io/), and running into issues with libdeflate not linking correctly. Using the system htslib would simplify things greatly.

luke-dt avatar Oct 26 '22 20:10 luke-dt

We would consider a PR if you would like to enable a build linking to an external libhts and libdeflate.

cjw85 avatar Oct 26 '22 20:10 cjw85

Hmm sorry for missing these replies. Indeed the PR is moot now by adding libdeflate to the required libraries.

Still, having the flexibility to hint which libraries (e.g. htslib) is great for users building/distributing software.

ccoulombe avatar Oct 28 '22 12:10 ccoulombe

I was still having issues with libdeflate, even when included as a link dependency. I got around this using the same trick as the bioconda recipe (directly patching the setup.py and build.py to disable the building of htslib), but agree that it would be nice to have the option as a user.

luke-dt avatar Oct 28 '22 15:10 luke-dt