cyvcf2 icon indicating copy to clipboard operation
cyvcf2 copied to clipboard

External htslib

Open Hoeze opened this issue 4 years ago • 12 comments

This pull request enables the usage of external htslib versions. Therefore, it applies this patch from bioconda and adds instructions how to use cyvcf2 with conda.

Hoeze avatar Mar 29 '20 19:03 Hoeze

Thanks for the PR. I think that @ccwang002 's PR in #143 might help in this regard . I don't want to remove the (default) static linking of htslib as it simplifies installation.

I think an approach like in #143 where an environment variable modifies the behavior would be better. I'd be interested to hear the thoughts of @ccwang002 and others.

brentp avatar Mar 29 '20 23:03 brentp

Does cyvcf2 work for htslib 1.10+? I think some int32 function signatures might have changed.

The dynamic linking works better in an "isolated" environment like bioconda, where it's guaranteed to find htslib. By the way, bioconda is fixing htslib to 1.9 for now. It's not clear to me when an end user will want to dynamically link htslib outside of bioconda. We also need to check if the dynamic linking works for wheels (pip by default will make one and cache)

Overall I think it's simpler to keep the static linking.

ccwang002 avatar Mar 30 '20 14:03 ccwang002

thanks for the input @ccwang002

yes, (backwards incompatible) changes would be required to support htsli

yes, (backwards incompatible) changes would be required to support htslib > 1.10.

this is good to know

yes, (backwards incompatible) changes would be required to support htslib > 1.10.

this is good to knowb > 1.10.

this is good to know, perhaps when bioconda moves to htslib >= 1.10, I will make the required changes for cyvcf2 and update at that time.

brentp avatar Mar 30 '20 15:03 brentp

Does cyvcf2 work for htslib 1.10+? I think some int32 function signatures might have changed.

The dynamic linking works better in an "isolated" environment like bioconda, where it's guaranteed to find htslib. By the way, bioconda is fixing htslib to 1.9 for now. It's not clear to me when an end user will want to dynamically link htslib outside of bioconda. We also need to check if the dynamic linking works for wheels (pip by default will make one and cache)

Overall I think it's simpler to keep the static linking.

My problem is exactly that fixing of htslib v1.9: I'm using other libraries in my environment that need at least htslib v1.10. So I can either use cyvcf2 or the other library which is annoying. Would it be a solution to stick to the PyPI-version (i.e. statically linked)?

@brentp: Bioconda always has the latest version of htslib (currently v1.10.2). I think new builds are automatically triggered when a new htslib version is being released. However, they won't update their htslib dependency as long as cyvcf2 does not officially support it.

Hoeze avatar Mar 30 '20 17:03 Hoeze

ok. i will update it to use htslib >= 1.10. Give me a couple weeks and feel free to ping me if it's not done by then.

brentp avatar Mar 30 '20 17:03 brentp

Thanks a lot @brentp!

Hoeze avatar Mar 30 '20 17:03 Hoeze

@ccwang002 does this seem ok to you? also, what do you think about making CYTHONIZE the default and using NO_CYTHONIZE instead?

brentp avatar Mar 30 '20 17:03 brentp

@Hoeze you are right that using the statically linked cyvcf2 from PyPI would be better in your case. bioconda currently pins the htslib to 1.9, but cyvcf2 can override it to 1.10.

If we default to CYTHONIZE, user will generate the C code every time they pip install, which is redundant and error-prone IMO. I think only a few people will want to build cyvcf2 from source that requires CYTHONIZE.

ccwang002 avatar Mar 30 '20 18:03 ccwang002

@ccwang002 As I understood, CYTHONIZE sounds like the solution to the bioconda problem: Whenever there's a new (supported) htslib version installed via conda, just re-run the cythonize to link to it, right? Otherwise, what would it take to make cyvcf2 more version-agnostic of htslib?

Hoeze avatar Mar 30 '20 18:03 Hoeze

That's right. The current bioconda build cythonizes anyway because it tries to dynamically link all the libraries. But to make cyvcf2 support both 1.9 and 1.10, it needs cyvcf2/cyvcf2.pxd and cyvcf2/helpers.h and maybe other files to have the function signature corresponding to 1.9 and 1.10 because they have incompatible APIs (mainly the conversions from int32_t to int64_t but I haven't looked at this carefully). It probably means extra work to maintain.

Given cyvcf2 is quite stable for a while, it's probably easier to drop 1.9 support in newer releases. People who needs 1.9 can just stay on 0.11.x.

ccwang002 avatar Mar 30 '20 18:03 ccwang002

I agree. When I update, I will drop support for < 1.10.

brentp avatar Mar 30 '20 19:03 brentp

I have this working here: https://github.com/brentp/cyvcf2/tree/htslib-1.10

if you test and verify it's working, I'll merge and tag a new release.

brentp avatar Apr 02 '20 14:04 brentp