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

Depend on libosqp instead of rebuilding it #48

Merged
merged 3 commits into from
May 31, 2023

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Nov 18, 2022

Upstream has been pushing new versions (0.7.x) that the bot seems to have missed.

Edit: as it turns out, these were not meant for publication anyway.

Edit²: Split off the version bump into #49, now just doing the switch to conda-forge's libosqp

Closes #37
Closes #36
Closes #35
Xref #25

@conda-forge-linter
Copy link

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

@marcelotrevisani
Copy link
Member

marcelotrevisani commented Dec 19, 2022

@conda-forge-admin please rerender

@h-vetinari
Copy link
Member Author

If you want we can separate this PR from the switch to libosqp, which is trickier than just a version bump

@h-vetinari h-vetinari force-pushed the revamp branch 4 times, most recently from be68d29 to 9b66469 Compare January 4, 2023 11:35
Copy link
Member Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think this would not look too badly if the python bindings didn't manage to include the osqp-sources twice, once as a submodule, and once as a CMake FetchContent.

Unfortunately, the bindings already rely on a header that only exists in that branch, which however does not have a proper tag. I opened an issue: osqp/osqp#497

recipe/patches/0005-point-to-our-own-builds.patch Outdated Show resolved Hide resolved
@conda-forge-webservices
Copy link
Contributor

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

@h-vetinari h-vetinari mentioned this pull request May 26, 2023
@h-vetinari h-vetinari force-pushed the revamp branch 2 times, most recently from f6e53f4 to 8906f24 Compare May 26, 2023 05:31
@h-vetinari h-vetinari changed the title Revamp Depend on libosqp instead of rebuilding it May 26, 2023
@h-vetinari h-vetinari force-pushed the revamp branch 4 times, most recently from 146214d to 2cd22d0 Compare May 26, 2023 09:41
@h-vetinari
Copy link
Member Author

Hey @conda-forge/osqp @conda-forge/libosqp
After being open for more than 2.5 years (xref #25), I've finally figured out how to build osqp against our existing shared library build of the C-side, libosqp (which contains libosqp.so / libosqp.dylib / osqp.dll etc. per platform).

That would finally bring this feedstock in line with conda-forge best practices, reduce the package footprint, and bring several other benefits.

However, this comes at the cost of having to deactivate the codegen ability of osqp. Basically:

  • To support codegen, we'd need to copy the sources of osqp and qdldl as data into the installed osqp package, so that it can be copied and compiled into a standalone library when needed.
  • We cannot sidestep this e.g. by pointing the codegen to our shared libosqp builds (aside from the fact that the end result would not be standalone anymore), because libosqp in conda-forged is compiled without the EMBEDDED symbol being defined, and has a different ABI than what the code generation here requires.
    • For example, I believe that substantial changes to src/osqp/codegen/utils.py would be necessary, e.g. to add status_polish, which is required by the libosqp type when EMBEDDED is not defined, see here
    • While trying to make this work nevertheless (i.e. depending on our libosqp for the main library, but keeping the source files as "data" in site-packages, copying them for codegen and running the result), I ran into segfaults (CI), due to namespace, symbol & ABI clashes about which library the python side of osqp is passing its values to, so this too fell on its face.

To recap the available options:

  1. patching the codegen to point to the shared libosqp is possible, but:
    • defeats the point of building standalone binaries
    • runs into ABI issues because our libosqp is compiled non-EMBEDDED
  2. copying the osqp/qdldl sources into $SP_DIR/osqp/codegen/sources is also possible, but:
    • creates segfaults when running the codegen tests
    • conceivable that renaming some artefacts (e.g. the internally compiled osqp) would work, but that's not user-friendly either
  3. creating a libosqp with the EMBEDDED ABI (there are three different ones actually!) is not reasonable
  4. leaving the status quo is not desirable either
  5. disable codegen (which I consider the least bad)

So I'm planning to disable codegen here (see the last patch), and wanted to ask if there are objections, alternatives I overlooked, or other thoughts about this.

CC @conda-forge/cvxpy @conda-forge/libqdldl @conda-forge/qdldl-python @vineetbansal @imciner2

@traversaro
Copy link

I think it is reasonable to disable codegen. If codegen is needed, one can always install the osqp wheels from PyPI in a conda environment.

@h-vetinari h-vetinari merged commit b23a85b into conda-forge:main May 31, 2023
@h-vetinari h-vetinari deleted the revamp branch May 31, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants