-
-
Notifications
You must be signed in to change notification settings - Fork 11
Add igraph dependency #97
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
Add igraph dependency #97
Conversation
…dependencies use a compatible C core.
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
|
@xylar , it would be good to get your take on this, given your previous involvement in #88. I think this added dependency is a reasonable alternative to ensure on incompatible C core can be used. Ideally we would address #88 of course, but as discussed, it might be better to have an upstream implementation of that. |
|
Good to keep in mind: Since we have experimental functions, python-igraph is guaranteed to be compatible only with the exact same igraph version that it vendors. This will be a bit cleaner after 1.0, but still, if a python-igraph vendors 1.1.x, it should not be allowed to try to link to either 1.0.x or 1.2.x. |
|
@vtraag , happy to take a look. I'm on vacation but will be back home this weekend. |
Thanks Szabolcs, that is well understood. We can ensure the version specification is updated for future python interfaces, and limit it to compatible minor releases.
Thanks! |
|
@vtraag, I'm finally getting a chance to look at this. I don't understand the internals but won't |
| - gmp | ||
| - texttable >=1.6.2 | ||
| - python | ||
| - igraph >=0.10,<0.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
igraph has a run_exports that knows about its compatibility:
https://github.com/conda-forge/igraph-feedstock/blob/main/recipe/meta.yaml#L18-L19
So we should move this to the host section and this will mean igraph is automatically also included as a run dependency with the appropriate constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another note here: python-igraph 0.10.x/0.11.x will NOT work with arbitrary igraph versions. If simply saying igraph >=0.10,<0.11 makes it possible for people to end up with an older igraph and a newer python-igraph, that WILL mess things up.
The options are:
- Specify a single compatible version of
igraph(not a range!) for each specific version ofpython-igraph - Stick to static linking for the igraph 0.10.x series (I suggest this)
- Take the risk of people ending up with broken configurations that may crash or produce invalid results.
This will be a little bit cleaner starting with igraph 1.x in that each python-igraph version should be compatible with an entire minor version range of igraph. However, in practice, there is likely not going to be more than one minor version in each series, unless we find a critical bug that needs immediate fixing. So in practice not much changes: each python-igraph version will be compatible with one igraph version.
Still, the intention is to be more rigorous about compatibility with the 1.x series, so I'd recommend using dynamic linking only for the 1.x series.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szhorvat, the run_exports from igraph itself will prevent this. My suggested approach would be that we restrict which version of igraph is allowed at build time and the version at runtime will be restricted to be newer than or equal to that version, and less that the next minor version (because of the x.x in the run_exports). If that is not correct, we should fix the run_exports in igraph itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a single compatible version of igraph (not a range!) for each specific version of python-igraph
We could do this but it would require ignoring the run_exports from igraph, which currently allow newer patch versions of igraph at runtime than were used a build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
With help from copilot, I have several suggested changes in c3208b9, which I'm testing in #98. They worked locally on linux64. If they work for other builds, I think we should cherry-pick that commit over here and rerender. If not, I'll try to debug there. The main thing is that we can clean things up a lot by using the external |
|
Alternatively, we could merge #98. |
Checklist
0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)This PR ensures that the C core
igraphis identical to the C core that is used by the Python interface. Without it, there could in principle be dependencies ofpython-igraphthat use an incompatible C core. This is especially relevant with the new igraph 1.0 C core release.Ideally, the C core is also directly used in builds, as discussed in #88. For now, this is a reasonable stopgap.