ENH: Use highspy in linprog

[HaoZeke]

Supersedes #18642.

Reference issue

Closes #15915. Closes #15888. Closes #19734.

What does this implement/fix?

• Replaces existing linprog Cython bindings with upstream highspy

Tests pass (locally anyway). However, there are still plenty of improvements which can be made (once highspy works a bit more robustly ERGO-Code/HiGHS#1405)

Additional information

Needs a PR over at highs (ERGO-Code/HiGHS#1460) and over at the SciPy fork (scipy/HiGHS#63).

Draft status until:

• [ ] Changes merged upstream in a new PyPI release Unrelated
• Be 100 % backwards compatible (or document changes)
• Fixup the enum translation (should be quick, just too tired rn)
• Fixup the commit history, early stages are derived from WIP: ENH: optimize.linprog: use highspy for _linprog_highs #18642 and should have co-commit credit for @mckib2
• Figure out / fix the xfail on Linux 32-bit
• Add the git submodule back
• Point to the scipy fork of highs

[HaoZeke]

Also @mckib2 if you'd like to take a look that'd be great (or maybe after its out of draft status).

[rgommers]

What's here looks good - lots of complexity gets removed. Hard to say more because an essential bit - actually building highspy as part of the SciPy build - seems to be missing.

[HaoZeke]

This is almost done, except the build process on the SciPy CI is stricter than over at HiGHs (w.r.t warnings) so some more work is required.

[rgommers]

You can silence classes of warnings by adding defining them (if they're not already present) here:

scipy/scipy/meson.build

Lines 287 to 300 in 21dcad2

	# C++ warning flags
	_cpp_Wno_cpp = cpp.get_supported_arguments('-Wno-cpp')
	_cpp_Wno_deprecated_declarations = cpp.get_supported_arguments('-Wno-deprecated-declarations')
	_cpp_Wno_class_memaccess = cpp.get_supported_arguments('-Wno-class-memaccess')
	_cpp_Wno_format_truncation = cpp.get_supported_arguments('-Wno-format-truncation')
	_cpp_Wno_non_virtual_dtor = cpp.get_supported_arguments('-Wno-non-virtual-dtor')
	_cpp_Wno_sign_compare = cpp.get_supported_arguments('-Wno-sign-compare')
	_cpp_Wno_switch = cpp.get_supported_arguments('-Wno-switch')
	_cpp_Wno_terminate = cpp.get_supported_arguments('-Wno-terminate')
	_cpp_Wno_unused_but_set_variable = cpp.get_supported_arguments('-Wno-unused-but-set-variable')
	_cpp_Wno_unused_function = cpp.get_supported_arguments('-Wno-unused-function')
	_cpp_Wno_unused_local_typedefs = cpp.get_supported_arguments('-Wno-unused-local-typedefs')
	_cpp_Wno_unused_variable = cpp.get_supported_arguments('-Wno-unused-variable')
	_cpp_Wno_int_in_bool_context = cpp.get_supported_arguments('-Wno-int-in-bool-context')

and then adding them to cpp_args for the relevant build target.

[rgommers]

It'd still be nice to get them fixed upstream, but that shouldn't be blocking for finalizing this PR.

[HaoZeke]

It'd still be nice to get them fixed upstream, but that shouldn't be blocking for finalizing this PR.

Thanks that helped, but I'm not sure of the best approach for the rest, it seems like using a shared library ends up with (expected) lookup errors, but the static library fails the wheel build. In both cases there's a too many public symbols error.

../build/subprojects/highs/highspy/_highs_options.cpython-310-x86_64-linux-gnu.so: too many public symbols!
000000000000d980 T PyInit__highs_options
0000000000031570 T _Z10first_wordRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEi
0000000000031240 T _Z10strToLowerPc
000000000002df90 T _Z11checkOptionRK15HighsLogOptionsRK15OptionRecordInt
000000000002e050 T _Z11checkOptionRK15HighsLogOptionsRK18OptionRecordDouble
000000000002cca0 T _Z11highsLogDevRK15HighsLogOptions12HighsLogTypePKcz
000000000002e130 T _Z12checkOptionsRK15HighsLogOptionsRKSt6vectorIP12OptionRecordSaIS4_EE
000000000002c8f0 T _Z12highsLogUserRK15HighsLogOptions12HighsLogTypePKcz
0000000000030290 T _Z12reportOptionP8_IO_FILERK15OptionRecordIntb13HighsFileType
000000000002fe00 T _Z12reportOptionP8_IO_FILERK16OptionRecordBoolb13HighsFileType
00000000000[30](https://github.com/scipy/scipy/actions/runs/6372457292/job/17295259302?pr=19255#step:11:31)660 T _Z12reportOptionP8_IO_FILERK18OptionRecordDoubleb13HighsFileType
0000000000030a90 T _Z12reportOptionP8_IO_FILERK18OptionRecordStringb13HighsFileType
0000000000030e90 T _Z13reportOptionsP8_IO_FILERKSt6vectorIP12OptionRecordSaIS3_EEb13HighsFileType
000000000002dc40 T _Z14boolFromStringNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERb
00000000000[31](https://github.com/scipy/scipy/actions/runs/6372457292/job/17295259302?pr=19255#step:11:32)4b0 T _Z14first_word_endRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEi
000000000002dd40 T
...

What is the best approach to dealing with libraries which (ideally) should be installed by the user? OR, I'm missing something about the way the static library should be linked, since other sub-libraries are linked statically (quadpack).

[rgommers]

The current version seems quite close to ready, modulo the use of a wrap file (I assume that's for convenience during development, and it'll change to a git submodule at the end) and this error:

meson-python: error: Could not map installation path to an equivalent wheel directory: '{libdir_static}/libhighs.a'

The static library linking looks fine here, the only problem is that the libhighs definition in the subproject has install: true and hence by default it's in the list of to-be-installed files, but that's not what we want. This fixes the wheel build:

python -m build -Cinstall-args=--skip-subprojects

To make that the default behavior, add this to pyproject.toml:

[tool.meson-python.args]
install = ['--skip-subprojects']

[rgommers]

Note that that feature does require Meson 1.2.0, so you should bump the minimum version in the top-level meson.build file as well.

[HaoZeke]

Thanks @rgommers, I'll get to updating this in a few days but:

... modulo the use of a wrap file (I assume that's for convenience during development, and it'll change to a git submodule at the end)

I was thinking we ought to keep it as a submodule? It should change to point to the Scipy/Highs instead of my own but other than that it would make more sense to keep the meson-first subproject approach. The only issue I can think of is that the highs repo ends up under subprojects which may be logically less than pleasant.

The way the build is structured now, the subproject defines all the necessary variables (including the files needed), which means we would very rarely need to update the meson.build in SciPy. e.g. With the submodule system, if a new file is added, we need to update the meson.build in SciPy. Under the current scheme, we'd continue as-is because it would be part of the variables exported from the subproject (we'd still need to update the wrap-file hash though).

Perhaps @eli-schwartz has more comments on submodules v/s subprojects?

[eli-schwartz]

Using it as a subproject is very much preferable. You get component isolation: it's not SciPy's job to be concerned about how highs gets built, you just want to automatically use whatever the highs subproject does to define its static-library artifact(s), and then you consume them.

This also unlocks the ability to switch from:

highs_proj = subproject('highs', ......

to

highs_proj = dependency('highs')

along with specifying in SciPy, default_options: ['wrap_mode=forcefallback']

This would allow downstream builders (especially linux distros) the option to build highs as an external package and then build SciPy against that. But people doing pip install scipy or using python dev.py build would by default use the subproject.

[rgommers]

That all sounds right - I didn't mean a git submodule instead of a subproject. Rather, I meant that I'm missing a git submodule under subprojects/. Or am I missing something obvious and that gets added automatically somehow? I haven't worked with wrap files before, but it seems to me like the sdist should contain a copy of HiGHS and the build should work offline after checking it out.

This would allow downstream builders (especially linux distros) the option to build highs as an external package and then build SciPy against that. But people doing pip install scipy or using python dev.py build would by default use the subproject.

Yes, that is what I had in mind as well.

[eli-schwartz]

meson dist --include-subprojects will create sdists that have the subproject included from the wrap file.

Whether to use subprojects via a wrap file or a git submodule (or both at the same time!) is largely unimportant as far as meson is concerned. The main thing that strikes me about the topic is that git submodules tend to be larger to download than a tarball, and, if you create "light" sdists that only download the subprojects with an online build when the person building the wheel hasn't disabled the use of subprojects, the tarball version works without git installed, while the submodule requires installing git.

I think both options are defensible choices, but adding a meson subproject as a git submodule will probably be more pleasant for you if you expect most people to interact with the source code via git using long-running clones, distribute sdists with subprojects included, and don't support GitHub autogenerated tag tarballs at all...

... which are all qualities that SciPy has.

[rgommers]

Thanks, that explains it. Then I had the right thing in mind:

modulo the use of a wrap file (I assume that's for convenience during development, and it'll change to a git submodule at the end)

The wrap file is indeed nicer now, when you're developing this PR while making fixes to HiGHS as well. And once it's done, you can swap the wrap file for a git submodule under subprojects/.

[HaoZeke]

Failures seem unrelated (cython_special) but I can reproduce them (with a newly created environment, not my older one) so I assume it has something to do with a dependency update (can't any obvious contenders though).

[rgommers]

but perhaps it might be more expedient to let it out into the wild as is

+1 for this. I haven't forgotten about this PR, it was just a busy period + holiday season. Planning to get this reviewed this week.

[rgommers]

This is very close. A few comments inline - all minor.

The silencing of unused variable warnings didn't work, because they're coming from the static library built within subprojects/highs:

[6/11] Compiling C++ object subprojects/highs/src/libhighs.a.p/lp_data_HighsSolve.cpp.o
../subprojects/highs/src/lp_data/HighsSolve.cpp:48:10: warning: unused variable 'imprecise_solution' [-Wunused-variable]
    bool imprecise_solution;
         ^
../subprojects/highs/src/l

Looks like those should be fixed upstream, and perhaps silenced in our forked repo in the meantime.

There are also a couple of other warnings. In the build:

ld: warning: ignoring duplicate libraries: '-lc++'

and in the configure stage:

highs| subprojects/highs/meson.build:16: WARNING: add_languages is missing native:, assuming languages are wanted for both host and build.

I'm seeing a number of other issues in highs/meson.build (mainly with compiler-specific flags that aren't check) that are likely to be a problem, so let me open a PR for those upstream.

I'll push a rebase to fix the conflict and get a fresh CI run.

[rgommers]

This works as intended it looks like; it basically checks out the git submodule automatically. The one minor issue is that it leaves behind an extra file:

subprojects/highs/.meson-subproject-wrap-hash.txt

If that's added to .gitignore then I think we're all good here.

[rgommers]

That's not actually true it seems - when building an sdist after using the auto-checkout-via-wrapfile, I see:

WARNING: Submodule 'subprojects/highs' is not checked out and cannot be added to the dist

So you still need to run:

$ git submodule update --init
Submodule 'subprojects/highs' (https://github.com/scipy/highs) registered for path 'subprojects/highs'

That is then instant. But the annoying thing is that git status no longer tells you that you've missed a step. You've got the right checkout, but haven't "connected" it as a submodule. Nothing much goes wrong though as far as I can tell, it's just a little weird.

[rgommers]

The linter is unhappy - the No explicit stacklevel keyword argument found at least look like they need to be fixed up.

[rgommers]

One other key thing here that I am still reviewing/testing more carefully is that with the use as a subproject, we get extra constraints from the upstream meson.build:

• minimum Meson version is set to 1.2.0
• optional dependency on zlib; looks like that is for file reading code only and we should disable it unconditionally when calling subproject('highs')
• usage of git for a version string in highspy (looks robust to failure, but still)
• a run_command('python3', '-c', ... that looks less robust
• logic for threads and libatomic dependencies that now needs to be kept in sync in two places (our own versions, and the copied ones in highs)
  • hasthreads_dep = dependency('threads', required: true while we have required: false. I'm not sure if it can fail on niche platforms (mingw is problematic IIRC).
• An unnecessary dependency on libm (our top-level meson.build explains why it's only needed for C)
• an explicit declare_dependency(link_args: '-lstdc++') that doesn't look right

So this isn't as close as I thought yet. I'd really like for our fork of highs not to diverge from upstream, so let's see how easy it is to patch things up there first.

[HaoZeke]

but perhaps it might be more expedient to let it out into the wild as is

+1 for this. I haven't forgotten about this PR, it was just a busy period + holiday season. Planning to get this reviewed this week.

Thanks for the comprehensive review and comments @rgommers, I'll address these both upstream and here this weekend.

[HaoZeke]

@rgommers I've addressed the issues raised on a PR to HiGHS here (sorry for the delay):
ERGO-Code/HiGHS#1603

Beyond that I've made a few cosmetic changes (rolling back black) and I think I've responded to all the comments (below):

• minimum Meson version is set to 1.2.0

  • Set to match SciPy's 1.1.0

• optional dependency on zlib; looks like that is for file reading code only and we should disable it unconditionally when calling subproject('highs')``

• usage of gitfor a version string inhighspy (looks robust to failure, but still)

  • Not used if called as subproject

• a run_command('python3', '-c', ... that looks less robust

  • Not used if called as a subproject

• logic for threadsandlibatomic dependencies that now needs to be kept in sync in two places (our own versions, and the copied ones in highs)

  • hasthreads_dep = dependency('threads', required: truewhile we haverequired: false. I'm not sure if it can fail on niche platforms (mingw is problematic IIRC).

• I'm not sure what can be done for this one, threads are a dependency of highs, and the libatomic fallback is required, not just for SciPy, so I've left it in

• An unnecessary dependency on libm(our top-levelmeson.build explains why it's only needed for C)

  • Only used if the C interface is built

• an explicit declare_dependency(link_args: '-lstdc++') that doesn't look right

  • Removed

• The linter is unhappy - the No explicit stacklevel keyword argument found at least look like they need to be fixed up.

  • The linter errors seems spurious, all the warn calls have stacklevel set explicitly, and the line flagged isn't that long
  • Can't reproduce locally with ruff lint scipy/optimize | grep highs

[HaoZeke]

Failing tests seem unrelated:

FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_method[uint8] - OverflowError: Python integer 504 out of bounds for uint8
FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_method[int8] - OverflowError: Python integer 184 out of bounds for int8
FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_rank3_valid[uint8] - OverflowError: Python integer 504 out of bounds for uint8
FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_rank3_valid[int8] - OverflowError: Python integer 184 out of bounds for int8
FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_rank3_same[uint8] - OverflowError: Python integer 504 out of bounds for uint8
FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_rank3_same[int8] - OverflowError: Python integer 184 out of bounds for int8
FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_rank3_all[uint8] - OverflowError: Python integer 504 out of bounds for uint8
FAILED scipy/signal/tests/test_signaltools.py::TestCorrelateReal::test_rank3_all[int8] - OverflowError: Python integer 184 out of bounds for int8
= 8 failed, 45923 passed, 2378 skipped, 147 xfailed, 16 xpassed in 342.04s (0:05:42) =
Error: Process completed with exit code 1.