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

Future-proof packaging of SyRI #217

Closed
wants to merge 4 commits into from

Conversation

lrauschning
Copy link
Contributor

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
Copy link
Contributor Author

(see also pypa/pip#12297)

@mnshgl0110
Copy link
Member

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
Copy link
Contributor Author

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 pushed a commit that referenced this pull request Feb 22, 2024
@mnshgl0110
Copy link
Member

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 #217

@mnshgl0110 mnshgl0110 closed this Feb 22, 2024
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.

None yet

2 participants