search

pysam-developers / pysam

Pysam is a Python package for reading, manipulating, and writing genomics data such as SAM/BAM/CRAM and VCF/BCF files. It's a lightweight wrapper of the HTSlib API, the same one that powers samtools, bcftools, and tabix.
https://pysam.readthedocs.io/en/latest/
MIT License
774 stars 274 forks source link

Move Cython to build dependencies in `setup.py` #1216

Closed althonos closed 1 year ago

althonos commented 1 year ago

Hi!

This PR moves Cython from the requires key to the setup_requires key in setup.py. With the current requirement, pysam would require Cython to be installed on the target machine, even when installing from a wheel, where no Cython compilation is necessary. By moving Cython to the setup_requires field, it indicates Cython is needed to build the package, but not actually needed to import the package once it has been built.

jmarshall commented 1 year ago

The requires entry does not cause Cython to be installed even when installing from a wheel — that would happen if we used install_requires, which we don't. There was however a bug in 0.21.0's pyproject.yaml that caused Cython to be unnecessarily installed, which has since been fixed: see #1186.

setup_requires is discouraged in favour of pyproject.toml, which we already use. So it would be better to remove this entry than to change it to setup_requires.

In fact, it doesn't really seem that this requires entry actually does anything any more anyway! It adds a Requires: cython (>=0.29.12) line to the resulting wheel's pysam-x.y.z.dist-info/METADATA file, but that's a v1.1 item that was superseded in 2005 and I suspect nothing actually acts on it now.

So it would be best to remove this line. If we do that, it would be best to update the setuptools requirement to v61.0.0, which is documented as the version that introduced robust pyproject.toml support. But I am in two minds whether to do that now or to wait until after the upcoming release…

jmarshall commented 1 year ago

Thanks for bringing this up. I've incorporated something based on your commit into PR #1219, so we can review these related changes together.

althonos commented 1 year ago

Okay thanks. I was just a bit surprised because a pip install of pysam would also install Cython, even when installing pysam from a wheel, so I figured there was something wrong with the dependencies:

image

jmarshall commented 1 year ago

Yes, that was the incorrect dependencies in v0.21.0's pyproject.toml, which were fixed in #1186.

I've now merged an adjusted version of this as e33a6993059b7d4a05c325ef25ab31bb504905a7.