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

more stringent cython version pin #1217

Closed ahvigil closed 1 year ago

ahvigil commented 1 year ago

The semi-recent release of cython 3 has done a number on a lot of my projects which pin older versions of pysam, due to various incompatibilities introduced with the major cython release. Furthermore, build isolation features in newer versions of pip mean its not enough to force a cython version before installing pysam to get it to install correctly, so a fairly common workflow for me nowadays is installing pysam with --no-build-isolation so I can manually install Cython<3

I think it might be a good idea for future releases of pysam to specify both lower and upper bounds on the required cython version. For example, the current requirement should be Cython>=0.29.12,<4 so that today's releases aren't rendered broken by future breaking changes in cython. Otherwise isolated pysam builds always try to use the newest cython.

jmarshall commented 1 year ago

Pysam was reasonably quick to support Cython 3, and the issues for older pysam versions were only minor. Nonetheless you are right that this would be worth doing, as long as it's made clear that there is no intrinsic bar on an eventual Cython 4 and the pysam of the day will be updating to be compatible with it.

However we go to some lengths to pre-cythonise our sdists, so for people installing via pip, Cython should not actually be necessary. The circumstances in which it would be necessary are when installing pysam for a new Python release that the Cython in use back when the pysam release was made did not support. In this case you would want to regenerate the *.c files with current Cython.

So ideally perhaps we would remove Cython from pyproject.yaml, while leaving it in requirements-dev.txt, which can be used by those (including readthedocs) building from a git repository, which does not contain the *.c files. But that might not be practical, as we would need to have Cython as an optional build requirement (which I am not sure is a thing); or perhaps the maintainers could regenerate existing release's sdists with new Cythons when necessary.

TL;DR we should add ,<4 in pyproject.toml, probably in requirements-dev.txt, but not in setup.py (cf https://github.com/pysam-developers/pysam/pull/1216#issuecomment-1712498809).

ahvigil commented 1 year ago

However we go to some lengths to pre-cythonise our sdists, so for people installing via pip, Cython should not actually be necessary. The circumstances in which it would be necessary are when installing pysam for a new Python release that the Cython in use back when the pysam release was made did not support. In this case you would want to regenerate the *.c files with current Cython.

thats good to know, and perhaps things have improved in the last few years. In my most recent experience I have a pipeline we're moving to python 3.9 that uses pysam 0.15 (I understand thats quite old but would rather handle updating that separately) and according to the comment at https://github.com/pysam-developers/pysam/issues/860#issuecomment-576194976 there are some quirks with 0.15.3 and 0.15.4 that caused my latest headache.

This is inspired by the functionality poetry provides in its caret requirement syntax which I've been using in other projects with a good amount of success.