package cylp

[h-vetinari]

Checklist

• Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
• License file is packaged (see here for an example).
• Source is from official source.
• Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
• If static libraries are linked in, the license of the static library is packaged.
• Build number is 0.
• A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
• GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
• When in trouble, please check our knowledge base documentation before pinging a team.

Saw a package I didn't yet know in the list of interfaces for cvxpy, and wanted to package it. Interestingly (AFAICT), the underlying library already has been packaged for conda-forge already, but not the python bindings.

CC @conda-forge/cvxpy @conda-forge/coin-or-cbc

[conda-forge-linter]

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/clpy) and found some lint.

Here's what I've got...

For recipes/clpy:

• Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

[conda-forge-linter]

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 (recipes/clpy) and found it was in an excellent condition.

[h-vetinari]

So it seems https://github.com/coin-or/CyLP/blob/master/setup.py installs into a custom library folder, and we'll need to patch this. Hopefully an optional fall-back to an environment variable LIBRARY_PREFIX (or similar) would also be accepted upstream as a patch.

Also, I just saw that windows is blocked on conda-forge/coin-or-cbc-feedstock#3

[h-vetinari]

Or perhaps, using COIN_INSTALL_DIR might suffice already...

[tkralphs]

I'm having trouble following what this package is trying to do. If you conda install coin-or-cbc, then pip install cylp should work out of the box. What is the advantage of a separate conda package? I also don't know what this comment is referring to.

[h-vetinari]

I'm having trouble following what this package is trying to do. If you conda install coin-or-cbc, then pip install cylp should work out of the box. What is the advantage of a separate conda package?

Hey @tkralphs, thanks for stopping by! Generally, mixing conda & pip is discouraged, so one definite advantage would be being able to install cylp completely without pip! Another benefit would be having precompiled packages across several platforms & python versions (currently there's only windows), without having to vendor the library, etc.

So then conda install cylp would also install coin-or-cbc automatically, but come with everything else that's needed too.

I also don't know what this comment is referring to.

Conda tries to package things independently and resolve vendored/static dependencies through the build recipe, so that artefacts don't get duplicated unnecessarily. Currently, I get errors trying to build cylp with conda, because setup.py installs into a non-default lib-folder that conda does not recognize as valid. You don't have to help me fix this, but I copied you as I thought the coin-or-cbc maintainers might be interested in knowing about (or even contributing to) the effort to prepare a very closely related package.

[tkralphs]

Hey @tkralphs, thanks for stopping by! Generally, mixing conda & pip is discouraged, so one definite advantage would be being able to install cylp completely without pip! Another benefit would be having precompiled packages across several platforms & python versions (currently there's only windows), without having to vendor the library, etc.

Hmm, guess I don't see the problem with mixing pip and conda in general, but I probabvly don't know what bad things can happen. It's worked for me so far. Anyway, it's just that I saw you removed coin-or-cbc as a dependency (or it appeared that way) and then added COIN_INSTALL_DIR, which should only be necessary in special circumstances, such as building yourself from source . If coin-or-cbc is conda installed already, I don't think it's needed. So it looked like the whole process was not being done as automagically as I would expect.

Conda tries to package things independently and resolve vendored/static dependencies through the build recipe, so that artefacts don't get duplicated unnecessarily. Currently, I get errors trying to build cylp with conda, because setup.py installs into a non-default lib-folder that conda does not recognize as valid. You don't have to help me fix this, but I copied you as I thought the coin-or-cbc maintainers might be interested in knowing about (or even contributing to) the effort to prepare a very closely related package.

Right, but I'm not aware that it is doing that and I don't see anything in setup.py that causes that. But anyway, happy to help. I'm not really an expert in working with conda, just try to help out as much as I can.

[h-vetinari]

Hmm, guess I don't see the problem with mixing pip and conda in general, but I probabvly don't know what bad things can happen. It's worked for me so far.

Conda needs its own packages to work properly - everything installed by pip might trample over what conda has carefully set up and break a given environment;

Anyway, it's just that I saw you removed coin-or-cbc as a dependency (or it appeared that way) and then added COIN_INSTALL_DIR, which should only be necessary in special circumstances, such as building yourself from source . If coin-or-cbc is conda installed already, I don't think it's needed.

conda always builds from source (or at least wherever possible); and since we want to reuse the existing coin-or-cbc build for conda-forge, we need to point the cylp installer in the right direction (at least as I understand what setup.py does).

So it looked like the whole process was not being done as automagically as I would expect.

The packaging effort is not automagic, there's an initial effort (which might be quite large for complicated packages) to get a package into the conda-ecosystem - this PR intends to do much of that initial work. Afterwards, things work almost completely automagically.

Right, but I'm not aware that it is doing that and I don't see anything in setup.py that cause that. But anyway, happy to help. I'm not really an expert in working with conda, just try to help out as much as I can.

That's because you're not familiar with conda build, which is completely OK. But basically, for conda to correctly package what gets produced in the build phase (so that it can be reproduced whenever requested in an environment), it looks for the created libraries/headers etc. not in an arbitrary directory, but in the environment-$PREFIX.

[h-vetinari]

Interestingly, the import of cylp succeeds, but fails for cylp.cy (and there are no obvious irregularities in the build...)

import: 'cylp'
import: 'cylp.cy'
Traceback (most recent call last):
  File "/home/conda/staged-recipes/build_artifacts/cylp-split_1621371078878/test_tmp/run_test.py", line 5, in <module>
    import cylp.cy
ModuleNotFoundError: No module named 'cylp.cy

[tkralphs]

Hey @h-vetinari, yes, I understand that the effort of actually doing the packaging is not automagic, what I meant was that the build itself should be. Looking at your build script, it does seem that all that is being done is a 'pip install', which is what I was expecting. I got the impression it was going to be more involved than that from your comments about patching, etc. Given that's what is done in the end, I'm still not quite grasping why doing that manually is a bad idea or why the result should be different, but don't feel like you need to explain further, I can do some Googling. You can, however, explain further what you meant by your comment that CyLP's setup.py uses a non-standard library directory. You implied something should be patched upstream, so could you just spell out what the patch would need to be to fix this? When I look at the build logs, I see everything going exactly as it does for me and the library being installed in exactly the same place. I didn't see that you did any patching.

In any case, I also couldn't see any obvious reason why import cylp.cy failed, but the build artifacts should tell the story if I could find them! The Azure Pipelines interface is pretty difficult to navigate.

[wolfv]

hmm, it looks like it properly copies the __init__

  copying cylp/cy/__init__.py -> build/lib.linux-x86_64-3.9/cylp/cy
... and later ...
  adding 'cylp/cy/CyWolfePivot.cpython-39-x86_64-linux-gnu.so'
  adding 'cylp/cy/__init__.py'
  adding 'cylp/cy/createCythonInterface.py'

[tkralphs]

Yes, everything seems to get installed properly...

[h-vetinari]

@tkralphs: I got the impression it was going to be more involved than that from your comments about patching,

The comment about patching was premature, at least before I found out about COIN_INSTALL_DIR, sorry. It may still be necessary (since pip install isn't working out of the box), but that's OK - ideally we find a way that improves both the upstream source code and the conda packaging.

@tkralphs: The Azure Pipelines interface is pretty difficult to navigate.

If you go to the top-level run overview, you see "1 published", but since the build didn't succeed, that actually corresponds to the skipped windows build.

otherwise if you want to try to run this locally, you could clone this branch, create a conda environment with conda-build in it, and then run conda build recipe/cylp

@tkralphs: Yes, everything seems to get installed properly...

It seems that the cylp/cy/__init__.py that gets created by the compilation gets overwritten by a direct copy afterwards? (thanks for spotting that @wolfv!)

[BastianZim]

How about explicitly importing the missing dependencies directly, either in the build or test section? That way you know if they're even available at all (Which they seem to be but apparently aren't).

I'll have a look at the or-tools feedstock...

From my (very) cursory glance it seems like you're doing everything that we did in or-tools so I'm not sure if that will reveal anything, unfortunately.

[BastianZim]

Maybe a slightly related problem: I have a separate PR #17080 where I'm also getting undefined symbol errors. The tests pass upstream so I would say that it's definitely related to the conda-forge setup. Not a solution but might make troubleshooting easier.

[BastianZim]

Hi everybody, not sure if this helps but my upstream fixed it in these PRs:

• Dependent libraries in libmcscf pyscf/pyscf#1185
• pyscf/pyscf@085e509

[h-vetinari]

Hm, I cannot see how the patch you posted relates to missing symbols @BastianZim?

    from .CyCoinIndexedVector import CyCoinIndexedVector
ImportError: [...]/lib/python3.7/site-packages/cylp/cy/CyCoinIndexedVector.cpython-37m-x86_64-linux-gnu.so: undefined symbol: _ZN17CoinIndexedVectorD2Ev

Could it be that we need to match compiler-options (like C++ standard or other ABI-relevant flags) between the coin-or-* packages and this PR?

[BastianZim]

To be honest, at that point I was just grasping at straws. The only reason I mentioned it is because it fixed it on my side but the two can be completely unrelated of course.

[h-vetinari]

@tkralphs: I spent some time trying to track down this error.

Looking some more at this answer, it sounds to me like the required libraries are not correctly declared in the build script (which isn't a Makefile or CMakelists.txt, but directly in setup.py), and indeed, it seems (at first glance) that this is not set correctly...

[h-vetinari]

OK, progress! 🥳

Imports succeed, now segfaulting in the test suite. This PR would hopefully benefit from conda-forge/coin-or-cbc-feedstock#14, so I'll probably wait for that.

[stale]

Hi friend!

We really, really, really appreciate that you have taken the time to make a PR on conda-forge/staged-recipes! conda-forge only exists because people like you donate their time to build and maintain conda recipes for use by the community.

In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of staged-recipes close excessively old PRs after six months. This PR will remain open for another month, and then will be closed.

If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on main so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the staged-recipes team.

Cheers and thank you for contributing to this community effort!

[h-vetinari]

Not stale

[stale]

Hi friend!

We really, really, really appreciate that you have taken the time to make a PR on conda-forge/staged-recipes! conda-forge only exists because people like you donate their time to build and maintain conda recipes for use by the community.

In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of staged-recipes close excessively old PRs after six months. This PR will remain open for another month, and then will be closed.

If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on main so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the staged-recipes team.

Cheers and thank you for contributing to this community effort!

[h-vetinari]

not stale

[conda-forge-webservices]

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 (recipes/cylp) and found it was in an excellent condition.

[h-vetinari]

Still passing the imports, still segfaulting (on both linux & unix) as soon as we hit the test suite.

CC @tkralphs

[tkralphs]

My bandwidth for looking into this is still pretty limited and my opinion is still that if this is useful for the community and we want to get it out there, we should just he happy with an import test for the time being. The package is working fine for the vast majority of people and it's clear that tests are failing in parts of the code that hardly anyone cares about.

Unfortunately, I have to allocate my open source development time wisely and given that CyLP now has wheels for most platforms and pip install works without needing to install any dependencies, it's hard to justify allocating time to fixing these tests in the short term.

[h-vetinari]

I mean, I'd be fine with skipping a couple niche broken tests, but that would be something that's failing in python. A segfault is a sign of a major problem, and I'm not in favour of publishing anything with issues of that magnitude.

It's great that cylp is installable through pip, yet those segfaults could very well be happening in the pip package as well (especially if installed in an environment with other conda-forge dependencies). If we cannot rely on a sensible test suite, we test nothing, and as the old programming adage goes: what's not tested is broken.

We've also known about the segfaults since almost 1.5 years already, so I don't know how to further underline the importance of this. It's in the interest of the users of cylp users (whether through pip or conda-forge) to fix this.

[stale]

Hi friend!

We really, really, really appreciate that you have taken the time to make a PR on conda-forge/staged-recipes! conda-forge only exists because people like you donate their time to build and maintain conda recipes for use by the community.

In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of staged-recipes close excessively old PRs after six months. This PR will remain open for another month, and then will be closed.

If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on main so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the staged-recipes team.

Cheers and thank you for contributing to this community effort!

[h-vetinari]

Not stale