Closed MrTomRod closed 2 years ago
The cfisher.c
file is outdated, but this builds after running cythonize
.
@MrTomRod : you can use the following package to install (fisher-0.1.9+1.g4bee476.dirty.tar.gz). I rebuilt the cfisher.c
file, and it builds fine with Python 3.10..
so IIUC, all that's required is to re cythonize cfisher.pyx with python 3.10 and it will resolve this issue (and still work on older versions) is that correct?
@brentp : exactly, just cythonizing cfisher.pyx
will work! (at least on supported Python versions, so 3.6+ I guess)
Otherwise, if you want, I can open a PR to generate the cfisher.c
file whenever someone installs the package (that's what i'm doing for instance in here) so that this issue doesn't appear again.
A PR that optionally cythonizes would be great!
For cyvcf2, we use the env var as e.g.: CYTHONIZE=1 python setup.py install
see here that would be ideal, I think.
Okay, I'll get that working then :+1:
Actually, you can make the cythonization mandatory if you add a pyproject.toml
file that list Cython
as a build dependency: in that case, you can assume Cython is alway present, cythonize all the time, and only distribute the pyx
file.
If you make the cythonization optional, any future version that breaks will need to be installed manually with CYTHONIZE=1 pip install fisher
, which is probably going to get you more errors in here :smiley:
@tanghaibao what do you think? Make cython mandatory?
I am personally in favor of reducing one dependency by not including cython there - yes I understand that it is a hassle when there are major python API changes but hopefully not too frequent and can usually be fixed right away like this one.
Oh is the issue in deployment as to what to include in pip install
when we have a conflicting .c
file to ship?
In that case, I think shipping only the .pyx
file and making cython mandatory may be the easiest here.
In fact, I just found that I did that for one of the packages that we had as well:
ok. so ok to simply require cython then, @althonos
thanks @tanghaibao
That fixed it! Fyi, I benchmarked your code against painyeph's/my hybrid code if you're interested: