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

Wheels for MacOS and Linux #175

Merged
merged 14 commits into from
Nov 10, 2020
Merged

Wheels for MacOS and Linux #175

merged 14 commits into from
Nov 10, 2020

Conversation

horta
Copy link
Contributor

@horta horta commented Nov 5, 2020

This should close #114.

Wheels built on TestPyPI: https://test.pypi.org/project/cyvcf2/#files

Platforms: MacOS, Linux
Python: 3.5, 3.6, 3.7., 3.8, 3.9

@horta horta mentioned this pull request Nov 5, 2020
6 tasks
@brentp
Copy link
Owner

brentp commented Nov 6, 2020

This looks great to me. @ccwang002 have any concerns?

@ccwang002
Copy link
Collaborator

Thank you for making this! This is very helpful. I tested the macOS wheels on python 3.8 and 3.9. Both wheels work for me.

I tested the python 3.8 linux wheel inside Docker. Reading VCF from the local file works, but the remote file fails. Here is how I tested it:

docker run -it --rm python:3.8-slim bash -l
pip --version
# pip 20.2.4 from /usr/local/lib/python3.8/site-packages/pip (python 3.8)
pip install https://test-files.pythonhosted.org/packages/9e/f2/746a6cd950ff8bdad6315d8a8244b4a7ec7723e623686a21c78d4b786f09/cyvcf2-0.20.10-cp38-cp38-manylinux2010_x86_64.whl
>>> from cyvcf2 import VCF
>>> v = VCF('https://github.com/brentp/cyvcf2/raw/master/cyvcf2/tests/test.vcf.gz')
[E::hts_open_format] Failed to open file "https://github.com/brentp/cyvcf2/raw/master/cyvcf2/tests/test.vcf.gz" : Input/output error
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cyvcf2/cyvcf2.pyx", line 257, in cyvcf2.cyvcf2.VCF.__init__
  File "cyvcf2/cyvcf2.pyx", line 189, in cyvcf2.cyvcf2.HTSFile._open_htsfile
OSError: Error opening https://github.com/brentp/cyvcf2/raw/master/cyvcf2/tests/test.vcf.gz

Would you be able to investigate this?

@ccwang002
Copy link
Collaborator

The linux fails is probably related to how the openssl is linked. If the openssl is linked to a different SSL or lives at the different location to the wheel building environment, cyvcf2 will fail to read all remote files. Like how cyvcf2 now links to htslib, we might need to statically link to openssl as well. We might be able to leverage pyca/cryptography's existing approach (for example, their wheel building workflow or their docker image).

Alternatively, we can just note that the pre-built wheel has limited functionality. Given not everyone uses cyvcf2 to read remote VCFs, it could be a good compromise. If the user really needs this, they should self compile everything (which works using the current instruction), or they should use bioconda.

@horta
Copy link
Contributor Author

horta commented Nov 10, 2020

Thanks @ccwang002 for the debugging!
I have now potentially fixed the issue. The root issue was that cyvcf2 was being built (on Linux) against ssl 1.0.x while we were testing it on machines with ssl 1.1.x machine.

I'm now installing openssl 1.1.x on the manylinux2014 image, then building and installing libssh2, curl lib, and then htslib.

I unfortunately cannot run Travis CI right now as my OSS credits is on red, and therefore I cannot use Travis do upload the wheels to test pypi.

If you have a look at this line:

python3 -m twine upload --repository testpypi --skip-existing wheelhouse/*

you might perhaps understand that I'm using the TWINE_USERNAME and TWINE_PASSWORD environment variables, defined in the Travis CI dashboard, to upload the wheels to test pypi. Is anyone willing to fork (or perhaps just merge) this and test upload to test pypi? Is there another solution while I don't have credits?

@brentp
Copy link
Owner

brentp commented Nov 10, 2020

pending @ccwang002 's approval, I am fine to merge as long as you continue to help with any additional required changes.

@horta
Copy link
Contributor Author

horta commented Nov 10, 2020

I'm happy with it, yes.

@ccwang002
Copy link
Collaborator

LGTM. Let's merge this and test again with the new wheels.

Once this works, we should also let CI automatically build (and upload) wheels whenever a new release is made.

@brentp brentp merged commit cf7991b into brentp:master Nov 10, 2020
@horta
Copy link
Contributor Author

horta commented Nov 12, 2020

Thanks a lot!

Just to clarify. How can we setup automatic upload to Test PyPI?

@horta
Copy link
Contributor Author

horta commented Nov 12, 2020

Also, do you have an ETA for next release? We can also try and build wheels for the current version of cyvcf2.

@brentp
Copy link
Owner

brentp commented Nov 12, 2020

I can tag a release any time. Can you walk me through what I need to do re wheels? (I just haven't kept up to date with python packaging stuff).

@horta
Copy link
Contributor Author

horta commented Nov 12, 2020

Sure. I would first deploy wheels to Test PyPI so we can make sure the new workflow works indeed.

The way I setup .travis.yml, it will try to deploy wheels (and the sdist) every time a new TAG is pushed to GitHub. It happens in two places (one for MacOS and one for Linux):

The line

python3 -m twine upload --repository testpypi --skip-existing wheelhouse/*

try to upload everything inside wheelhouse, which happens to be wheels only. The line

python3 -m twine upload --repository testpypi --skip-existing dist/*

does the same but with the source distribution (.tar.gz).

It will only work if you had defined the environment variables TWINE_USERNAME and TWINE_PASSWORD in Travis CI: https://travis-ci.com/github/brentp/cyvcf2/settings (their values won't show up in travis logs).

@hammer
Copy link

hammer commented Nov 30, 2020

@brentp is there any chance the wheels will be included in a cyvcf2 release soon? We‘d love to make use of them in https://github.com/pystatgen/sgkit!

@brentp
Copy link
Owner

brentp commented Nov 30, 2020

thanks @horta for clear description (and @hammer for reminder). i just pushed a new tag after updating those env vars.
if that appears to work, let me know and i'll change testpypi to (i assume?) pypi.

@brentp
Copy link
Owner

brentp commented Dec 1, 2020

seems to be issues with travis-ci right now with using up my free quota. thought it would reset on today, december 1. but that doesn't seem to be the case yet.

@horta
Copy link
Contributor Author

horta commented Dec 1, 2020

It might be possible to buy some credits: https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing
@hammer

@hammer
Copy link

hammer commented Dec 3, 2020

@brentp any luck with Travis credits? If not, I'd be happy to buy some for you to get these wheels published!

@brentp
Copy link
Owner

brentp commented Dec 3, 2020

I have an email out to travis-ci with no response yet. Not sure why the credits didn't reset. Hopefully we'll hear from them soon. If not, I'll either do it manually or take you up on that. Looks like $69/month https://travis-ci.com/account/plan

@horta
Copy link
Contributor Author

horta commented Dec 3, 2020

I should have tried GitHub actions. I did on Travis CI because I have more experience with... (and they didn't have those limits =P)

@hammer
Copy link

hammer commented Dec 8, 2020

I see a release was made on December 5 but it does not appear that the wheels made it? https://pypi.org/project/cyvcf2/#files. My apologies if I'm missing something!

@brentp
Copy link
Owner

brentp commented Dec 8, 2020

Yes, the CI is not running (https://news.ycombinator.com/item?id=25338983)
We'll have to switch to using github actions.

@horta
Copy link
Contributor Author

horta commented Dec 8, 2020

But it didn't trigger a Travis building (https://travis-ci.com/github/brentp/cyvcf2/builds) probably because of lack of OSS credits.

@hammer
Copy link

hammer commented Dec 15, 2020

We'll have to switch to using github actions.

Should we file an issue to track this work? Would you like help with that?

@brentp
Copy link
Owner

brentp commented Dec 15, 2020

yes! please!

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.

Wheel distribution
4 participants