search

schneebergerlab / syri

Synteny and Rearrangement Identifier
https://schneebergerlab.github.io/syri/
MIT License
303 stars 36 forks source link

Future-proof packaging of SyRI #217

Closed lrauschning closed 4 months ago

lrauschning commented 8 months ago

This PR changes the packaging of SyRI to support installing both with python setup.py install and pip install . by adding a pyprojects.toml. Also, the cython version has been pinned to 0.29 as trying to compile with cython 3.x was causing an error. I think this might be an issue with cython, as it cythonizes successfully and then causes a compile error during some macro expansion when the cpp code is compiled with GCC.

lrauschning commented 8 months ago

(see also pypa/pip#12297)

mnshgl0110 commented 6 months ago

I am not a fan of using a ceiling for the version of dependencies (cython in this case). I assume that the issue with cython would be happening with other repos as well and therefore either cython would be updated in future to fix this bug or we would need to update how we use cython to make it future proof. Relying on old versions of dependencies does not feel nice. We would need to figure out what exactly is causing this error and what are the cython developers' response to tackle that.

I liked the use of entry_point, that's super cool. Have you checked that it works as intended. I would have to find time to do that myself.

The PR also include some code-cleanup and redundancies removal. But they were there to assist in debugging and to make things more explicit. So, I think, it would be better to not remove them.

lrauschning commented 6 months ago

I am not a fan of using a ceiling for the version of dependencies (cython in this case). I assume that the issue with cython would be happening with other repos as well and therefore either cython would be updated in future to fix this bug or we would need to update how we use cython to make it future proof. Relying on old versions of dependencies does not feel nice. We would need to figure out what exactly is causing this error and what are the cython developers' response to tackle that.

Neither am I; I was planning on looking into this more closely and report it upstream to Cython[0], but didn't find the time. As a temporary stopgap while we report it I think it makes sense to pin the dependency; after this is fixed in Cython, we would need to set the dependency to after the fix is merged anyway.

I liked the use of entry_point, that's super cool. Have you checked that it works as intended. I would have to find time to do that myself.

Yes, though it takes some getting used to the new packaging has some cool features. I tested it by installing syri and running it on some Ampril genomes at the time if I recall correctly, everything seemed to be working – I wouldn't expect a change in packaging to change anything in a big way anyways.

The PR also include some code-cleanup and redundancies removal. But they were there to assist in debugging and to make things more explicit. So, I think, it would be better to not remove them.

Sure, feel free to cherry-pick the commits/commit to this branch!

[0] I looked into it at the time, and it was some kind of an issue in a macro in the generated C++ to do with some includes if I recall correctly, so I think it's an issue with Cython rather than syri.

mnshgl0110 commented 4 months ago

It seems the issue with cython is cause by star imports. That is if a cython module uses from <some_module> import *. Removing them seems to be solving the issue. I have done that for couple of culprit files and it seems to be working now. Have also added the pyproject.toml in https://github.com/schneebergerlab/syri/pull/217