Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements to Compilation Process, CI/Wheel Workflow, and Bug Fixes #290

Merged
merged 9 commits into from
Dec 19, 2023
Merged

Enhancements to Compilation Process, CI/Wheel Workflow, and Bug Fixes #290

merged 9 commits into from
Dec 19, 2023

Conversation

graphenn
Copy link
Contributor

@graphenn graphenn commented Dec 15, 2023

  • Direct Static/Dynamic Linking to htslib and Improved Maintainability
    Update cyvcf2 to directly link with the htslib library, both statically and dynamically, eliminating the need to compile htslib from its source code. This decoupling offers better maintainability, enabling cyvcf2 to fully use the optimization of htslib's native configure for compilation options. Consequently, this change results in a reduction of compilation time by approximately one-third.
    • End-users can continue to install cyvcf2 using pip install cyvcf2, and there are no changes required on their end. There is a minor modification in the dependency, moving from libssl to libcrypto, because htslib depending on libcrypto not libssl. However, since the libssl package also includes the libcrypto library, this transition will be seamless for users.

    • New introduced support for using the system's htslib library during pip installation (should be installed from source code, --no-binary) by adding the environment variable CYVCF2_HTSLIB_MODE=EXTERNAL. This allows users to leverage their existing htslib installation.

    • Developers can follow the previous approach of compiling htslib first and then cyvcf2 or directly compile cyvcf2, which will handle the htslib compilation process automatically.

    • A new command, has been added to facilitate the cleaning of both htslib and cyvcf2 compilation files simultaneously

python setup.py clean_ext
  • Restructured CI/Wheel Workflow for New Compilation Process
    To adapt to the new compilation methodology, rebuilt the CI/Wheel workflow:

    • Revised the workflow to align with the new compilation process

    • Add build test for musllinux (alpine), PyPy3.10, and Windows(MSYS2 only, Experiment)

    • Wheel new supports musllinux, Linux ARM64(manylinux/musllinux), and PyPy3.10

  • bump htslib version to 1.19

  • Fix some crash bugs

@brentp
Copy link
Owner

brentp commented Dec 15, 2023

This looks great from what I can tell!
Perhaps @ccwang002 or @tomwhite can have a look given the scale of changes.

Copy link
Collaborator

@tomwhite tomwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to review in detail, but it looks good to me.

@ccwang002
Copy link
Collaborator

Great work and thank you for addressing many pain points all at once 🙌🏼

The changes look good to me. I also tested the AWS S3 VCF reading and everything works as expected. Please see my additional feedback below:

  1. Looks like we can greatly simplify the current bioconda build process using CYVCF2_HTSLIB_MODE=EXTERNAL
  2. We should update the README with the latest building instructions and the compatible htslib versions (is it 1.19+?)
  3. Is there a way to check the version of the linked htslib? I am thinking something like a function call get_htslib_version()

@graphenn
Copy link
Contributor Author

As I test, htslib 1.12/1.15/1.18/1.19 are compatible, I can't compile 1.11 and 1.10 on my computer.
Get htslib version can refer to samtools version long_version()

@brentp
Copy link
Owner

brentp commented Dec 19, 2023

I will add the version stuff. Thanks very much @graphenn .

@brentp brentp merged commit d2a39e8 into brentp:main Dec 19, 2023
12 checks passed
@graphenn graphenn deleted the build branch December 25, 2023 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants