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

ENH: optimize: integrate the PRIMA library for COBYLA #20964

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nbelakovski
Copy link
Contributor

@nbelakovski nbelakovski commented Jun 15, 2024

Reference issue

Closes gh-18118

What does this implement/fix?

This integrates the PRIMA library for derivative free optimization. The primary goal of this PR is to replace the existing F77 implementation of COBYLA with a modern Fortran implementation, but by integrating PRIMA we gain access to 4 other derivative free optimizers: NEWUOA and UOBYQA for unconstrained problems, BOBYQA for bound constrained problems, and LINCOA for linearly constrained problems. We can add these to minimize in a future PR if we so desire, but again the main focus here is on replacing a F77 version of an algorithm with a modern implementation for the purposes of reducing maintenance burden associated with old Fortran code.

Additional information

There were quite a few changes to make so I used several commits to break apart the changes and make them slightly more reviewable.

Commit #1: Adds PRIMA and uses it to replace COBYLA with the minimal amount of changes necessary. It does not make changes needed to support the new features of the new COBYLA implementation like support for linear constraints (although I did make changes to support handling bounds directly because that was easy) or new solver options. I also modified one of the tests in test_cobyla because it was very confusing and the new implementation broke the test by performing better than the old implementation.

Commit #2: Removes the old COBYLA implementation.

Commit #3: Makes changes to allow COBYLA to accept new-style constraint objects, including changes to documentation and tests.

Commit #4: Adds support, tests, and documentation for a callback function which accepts IntermediateResult
Commit #5: Adds support, tests, and documentation for the ftarget option
Commit #6: Adds support and documentation for the iprint option (a test already existed for a more limited version of this option).

@github-actions github-actions bot added scipy.optimize scipy._lib Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org Fortran Items related to the internal Fortran code base Meson Items related to the introduction of Meson as the new build system for SciPy labels Jun 15, 2024
@lucascolley lucascolley removed the Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org label Jun 15, 2024
@lucascolley lucascolley changed the title Integrate PRIMA into SciPy for COBYLA ENH: optimize: integrate the PRIMA library for COBYLA Jun 15, 2024
@lucascolley lucascolley added the enhancement A new feature or improvement label Jun 15, 2024
@nbelakovski
Copy link
Contributor Author

Looks like there are a few tests that I forgot to run locally. I'll work on those over the next couple of days. Some of them are not very trivial, since PRIMA handles nan's in a very different way, and also it calls the callback once per iteration instead of once per function evaluation so there's a solid handful of changes to be made for those. In the meantime I welcome review and suggestions, I hope I've done the meson part decently well. I mostly modeled it on HIGHS.

@nbelakovski nbelakovski force-pushed the prima branch 3 times, most recently from d1b2029 to 681d35b Compare June 16, 2024 00:25
@rgommers
Copy link
Member

Thanks @nbelakovski! At first sight, this looks excellent.

Quick question about libquadmath: is there a downside to leaving that optional dependency out? It seems like differing behavior based on whether it's available or not is undesirable. Most of our testing is with gfortran (which will have libquadmath available), so it's also risking lack of test coverage when we use a different Fortran compiler.

@nbelakovski
Copy link
Contributor Author

nbelakovski commented Jun 16, 2024

Quick question about libquadmath: is there a downside to leaving that optional dependency out? It seems like differing behavior based on whether it's available or not is undesirable. Most of our testing is with gfortran (which will have libquadmath available), so it's also risking lack of test coverage when we use a different Fortran compiler.

I only needed to put that in due to some compiler issues. On Windows it was complaining about quadmath_snprintf not being defined. numpy had a similar issue and solved it similarly. There's no direct reference to quadmath in the code as far as I can tell.

Edit: To clarify, Windows complained about undefined symbols from quadmath, hence putting it in as a dependency, but then macOS complained that it couldn't find it, hence setting required to false.

@dschmitz89
Copy link
Contributor

This is awesome, thanks a lot @nbelakovski !

Could you provide the local optimizer benchmark results comparing the new old and new COBYLA versions? In CI the results can for example be found here.

@rgommers
Copy link
Member

rgommers commented Jun 19, 2024

There are a lot of CI failures to work through. I had a quick look:

  • The build warnings caught by -Werror seem straightforward to fix.
    • Note that ideally PRIMA would add that check in one of its own CI jobs itself, to avoid new errors creeping in that we'd then see the next time we try to upgrade.
  • The linter is unhappy with some emoji characters and other esoteric unicode ones. If they're necessary in the tests, we can allow them. If not, probably better to clean them up in PRIMA itself.
  • The segfault in the musllinux job needs fixing.
  • The warning in the doc build should be an easy fix: (optimize.minimize-cobyla.rst:38: WARNING: Block quote ends without a blank line; unexpected unindent.)
  • The four test failures in the 32-bit job look like there is an actual precision issue in the new implementation.

@nbelakovski
Copy link
Contributor Author

nbelakovski commented Jun 19, 2024

  • The build warnings caught by -Werror seem straightforward to fix.
    • Note that ideally PRIMA would add that check in one of its own CI jobs itself, to avoid new errors creeping in that we'd then see the next time we try to upgrade.

I have a PR opened in PRIMA to fix these. I think @zaikunzhang is busy with other things so I don't know when he'll get to it. For now I've pointed the PRIMA submodule against my repo with the branch containing the fixes. When @zaikunzhang is able to approve the PR we can re-point back to the main PRIMA repo.

  • The linter is unhappy with some emoji characters and other esoteric unicode ones. If they're necessary in the tests, we can allow them. If not, probably better to clean them up in PRIMA itself.

These are coming from pybind11, which is a submodule in PRIMA. I tried to set recurseSubmodules to false in .gitmodules to prevent this submodule from being included, but that didn't work. Ultimately I explicitly set submodule to false in lint.yml which feels like an appropriate solution (it should have never had recursive I think, since that implies linting other projects which may have their own conflicting linting setups).

This also brings up a small issue that PRIMA expects to be compiled with a particular version of pybind11 (i.e. the one it includes as a submodule) and it will be compiled with a different version in the SciPy build process if those versions aren't the same. I expect this will "just work" most of the time, but when it doesn't we can either try to synchronize the versions or apply some patches.

  • The segfault in the musllinux job needs fixing.

My notes in the PRIMA repo say that we disabled this build because while it successfully compiled, there was an error when testing. I'll take a look but this could be a problem.

  • The warning in the doc build should be an easy fix: (`optimize.minimize-cobyla.rst:38: WARNING: Block quote ends without a blank line; unexpected unindent.)

Fixed

  • The four test failures in the 32-bit job look like there is an actual precision issue in the new implementation.

This one is also giving me some trouble. x0 starts very close to the solution, and yet PRIMA struggles to move it closer to the solution than 1e-3 difference. It's also struggling with the bounds test. I spent some time looking into this today but haven't gotten anywhere, I will keep looking at it.


So bottom line for today is that 3/5 of the issues brought up by @rgommers should be fixed and 2/5 require further fine-tuning/investigation.

@nbelakovski nbelakovski force-pushed the prima branch 3 times, most recently from a450708 to caafbda Compare June 19, 2024 23:21
@nbelakovski
Copy link
Contributor Author

I've tracked down the issue with musl to an issue using nested function in Fortran code. I've created a minimum reproducible example and opened a discussion in Fortran discourse: https://fortran-lang.discourse.group/t/nested-functions-segfault-with-musl/8243

This is not easy to work around. COBYLA itself uses a nested function in the new implementation, and we rely on nesting functions for the Fortran<->C interface to work.

We'll see if the Fortran discourse comes up with anything. They're usually quite active and helpful.

@TiborGY
Copy link

TiborGY commented Jun 22, 2024

I've tracked down the issue with musl to an issue using nested function in Fortran code. I've created a minimum reproducible example and opened a discussion in Fortran discourse: https://fortran-lang.discourse.group/t/nested-functions-segfault-with-musl/8243

This is not easy to work around. COBYLA itself uses a nested function in the new implementation, and we rely on nesting functions for the Fortran<->C interface to work.

We'll see if the Fortran discourse comes up with anything. They're usually quite active and helpful.

I have not looked at your issue in detail, but in the past when I had issues with errors I could not explain, compiling all Fortran code with -frecursive fixed it. For Intel compilers I used "-assume recursion -reentrancy threaded". Could be worth a try.

@nbelakovski
Copy link
Contributor Author

I have not looked at your issue in detail, but in the past when I had issues with errors I could not explain, compiling all Fortran code with -frecursive fixed it. For Intel compilers I used "-assume recursion -reentrancy threaded". Could be worth a try.

I tried a variety of command line options in the vain hope that one of them would work but no joy. I did ask over on the musl mailing list about it and I have good news and bad news. The good news is that there's a new command line option in gcc 14 that should help, -ftrampoline-impl=heap, the bad news is a) it's in gcc 14 which is pretty new, not many platforms have packages for this yet and b) because of a) I haven't been able to validate that this actually works.

I'll take another looks at the PRIMA code and see if we can't rework things to avoid needing a nested function, but for the moment it looks like it's a blocker until gcc 14 is more widely available.

@rgommers
Copy link
Member

Nested functions do look like a problem indeed. Known issue apparently: https://fortran-lang.discourse.group/t/is-creating-nested-subroutines-functions-considered-good-practice-in-fortran/6545/26. So it's not well-supported by gfortran and also problematic in other compilers (except LFortran it looks like). The "executable stack not allowed" is a valid and common security argument, so it's not even specific to Musl but may cause issues elsewhere too. It may also be a problem for building with Intel Fortran (currently not tested in our CI, but will be soon), as well as for Pyodide (which uses f2c to get Fortran code to run).

Nested functions are a language feature in Fortran, but only a GNU-specific extension for C and not allowed in C++ (except for lambdas) - which may explain the spotty support. It looks like the language feature isn't well-supported and hence best avoided.

@nbelakovski
Copy link
Contributor Author

Thanks for adjusting the check_installation script! Would you mind breaking that out into a separate PR?

Done: #21493

It's probably easiest to merge that one first and then I can rebase this one.

@dschmitz89
Copy link
Contributor

The CI issues should be fixed. We'll have to wait for all the tests to run to confirm. Here are the results of benchmarking compared to main

Some tests are worse, some are better. There's one outlier in test №27, but looking at the link provided by @dschmitz89 in this comment, we see the older COBYLA having a similarly poor result on №27, so it's probably just sensitive to different implementations of floating point arithmetic (unfortunately that is consistent with my experience with this algorithm, it does seem to be quite sensitive to small changes in floating point values. I even had to modify the starting points of some tests by 1e-3 so that they would pass for 32-bit). A similar story can found for test №17 which is also an outlier. Overall though 22 tests are worse while 39 tests are better so it looks like an improvement on balance.

Edit: sigh it looks like tests are failing on Mac due to the change in starting position that I made in order for the tests to succeed on 32-bit. I guess I'll have to play a little bit of whac-a-mole to find an initial point that can pass all tests. I'm open to other ideas if you all have any.

Thanks @nbelakovski . On average, the new implementation looks better, that is great to see. I am wondering if both versions use the same convergence criteria as we sometimes have very different final objective function values.

Copy link
Contributor

@dschmitz89 dschmitz89 left a comment

Choose a reason for hiding this comment

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

Thanks again @nbelakovski . This looks close. The most important blocker for me currently is the choice of tol? I know that this is often a matter of preference, but could we compare the performance of COBYLA using for example 1e-4, 1e-5, ... 1e-8?

CC @zaikunzhang in case you have a strong opinion here as the expert on this algorithm.

scipy/optimize/_cobyla_py.py Outdated Show resolved Hide resolved
scipy/optimize/_cobyla_py.py Outdated Show resolved Hide resolved
@nbelakovski
Copy link
Contributor Author

Comments about tolerance and f_target have been addressed and I've rebased on main to incorporate the changes to tools/check_installation.py, so I think this is ready to go!

Copy link
Contributor

@dschmitz89 dschmitz89 left a comment

Choose a reason for hiding this comment

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

Thanks @nbelakovski . LGTM. Let's wait for more reviews before merging still.

@dschmitz89
Copy link
Contributor

@andyfaff would you have a look here? We use the maintained modern Fortran version of COBYLA via its C interface, does that resolve your concerns about Fortran?

@andyfaff
Copy link
Contributor

andyfaff commented Sep 14, 2024

This is less of an issue. Using prima to replace the existing COBYLA is a win, because the existing code is poor. Going forward I really don't think we should introduce more Fortran, unless it's replacing hard to maintain code. We had a discussion on Fortran related stuff a few weeks back, but I can't remember where it was.

Edit: chat on Fortran inclusion was on Slack

@nbelakovski
Copy link
Contributor Author

So at this point are we just waiting for review from @rgommers ?

@lucascolley lucascolley mentioned this pull request Sep 17, 2024
36 tasks
@rgommers
Copy link
Member

So at this point are we just waiting for review from @rgommers ?

I've been mostly out of action for weeks now, and still not quite back. Posting a review shortly to help unblock things hopefully, but my iteration time here is likely to be slow.

Note that I triggered the CirrusCI aarch64 job just now, and it's unhappy:

Cloning into '/tmp/cirrus-ci-build/scipy/_lib/prima/.development'...
Host key verification failed.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
fatal: clone of '[email protected]:libprima/prima_development.git' into submodule path '/tmp/cirrus-ci-build/scipy/_lib/prima/.development' failed
Failed to clone '.development'. Retry scheduled

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

I did a bit of testing. Building locally on macOS with the conda-forge compilers I'm seeing a large number of linker warnings (also visible in the macOS conda CI job) like:

[1338/1339] Linking target scipy/optimize/_prima/_prima.cpython-312-darwin.so
ld: warning: could not create compact unwind for ___bobyqa_c_mod_MOD_calfun: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___bobyqa_c_mod_MOD_callback_fcn: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for _bobyqa_c: register 73 saved somewhere other than in frame

Full output:

[1338/1339] Linking target scipy/optimize/_prima/_prima.cpython-312-darwin.so
ld: warning: could not create compact unwind for ___bobyqa_c_mod_MOD_calfun: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___bobyqa_c_mod_MOD_callback_fcn: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for _bobyqa_c: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___cobyla_c_mod_MOD_calcfc: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___cobyla_c_mod_MOD_callback_fcn: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for _cobyla_c: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___lincoa_c_mod_MOD_calfun: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___lincoa_c_mod_MOD_callback_fcn: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for _lincoa_c: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___newuoa_c_mod_MOD_calfun: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___newuoa_c_mod_MOD_callback_fcn: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for _newuoa_c: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___uobyqa_c_mod_MOD_calfun: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___uobyqa_c_mod_MOD_callback_fcn: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for _uobyqa_c: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___bobyqa_mod_MOD_bobyqa: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___cobyla_mod_MOD_cobyla: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___lincoa_mod_MOD_lincoa: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_character: register 28 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_imatrix: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_ivector: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_lvector: register 26 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_rmatrix_qp: registers 25 and 26 not saved contiguously in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_rvector_qp: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_rmatrix_dp: registers 25 and 26 not saved contiguously in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_rvector_dp: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_rmatrix_sp: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___memory_mod_MOD_alloc_rvector_sp: registers 23 and 24 not saved contiguously in frame
ld: warning: could not create compact unwind for ___newuoa_mod_MOD_newuoa: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___uobyqa_mod_MOD_uobyqa: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___bobyqb_mod_MOD_errbd.isra.0: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___bobyqb_mod_MOD_bobyqb: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___cobylb_mod_MOD_getcpen.constprop.0.isra.0: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___cobylb_mod_MOD_cobylb: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___debug_mod_MOD_errstop: register 28 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___debug_mod_MOD_validate: register 24 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___evaluate_mod_MOD_evaluatefc: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___evaluate_mod_MOD_evaluatef: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___evaluate_mod_MOD_moderatec: register 28 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___evaluate_mod_MOD_moderatex: register 28 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___history_mod_MOD_rangehist: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___history_mod_MOD_savehist: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_isbanded.constprop.0: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_trueloc.constprop.0: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_eigmin_sym_trid: registers 76 and 77 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_linspace_i: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_linspace_r: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_maximum2: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_maximum1: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_minimum2: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_minimum1: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_falseloc: register 28 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_trueloc: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_sort_i2: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_sort_i1: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_named_norm_mat: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_named_norm_vec: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_p_norm: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_issymmetric: registers 25 and 26 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_planerot: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_hypotenuse: registers 19 and 20 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_qr.constprop.0: register 79 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_project1: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_project2: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_diag: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_hessenberg_hhd_trid: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_lsqr_rdiag: register 77 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_isinv: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_inv: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_solve: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_hessenberg_full: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_matprod22: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_matprod21: register 28 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_r2: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___linalg_mod_MOD_r1: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___lincob_mod_MOD_lincob: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___newuob_mod_MOD_newuob: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___preproc_mod_MOD_preproc: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___string_mod_MOD_int2str: register 26 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___string_mod_MOD_real2str_scalar: registers 25 and 26 not saved contiguously in frame
ld: warning: could not create compact unwind for ___uobyqb_mod_MOD_uobyqb: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___checkexit_mod_MOD_checkexit_con: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___checkexit_mod_MOD_checkexit_unc: registers 25 and 26 not saved contiguously in frame
ld: warning: could not create compact unwind for ___geometry_bobyqa_mod_MOD_geostep: registers 76 and 77 not saved contiguously in frame
ld: warning: could not create compact unwind for ___geometry_bobyqa_mod_MOD_setdrop_tr: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___geometry_cobyla_mod_MOD_geostep: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___geometry_cobyla_mod_MOD_setdrop_tr: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___geometry_lincoa_mod_MOD_geostep: register 77 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___geometry_lincoa_mod_MOD_setdrop_tr: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___geometry_newuoa_mod_MOD_circle_fun_biglag: register 24 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___geometry_newuoa_mod_MOD_geostep: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___geometry_newuoa_mod_MOD_setdrop_tr: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___geometry_uobyqa_mod_MOD_geostep: register 79 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___geometry_uobyqa_mod_MOD_setdrop_tr: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___initialize_bobyqa_mod_MOD_inith: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___initialize_bobyqa_mod_MOD_initq: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___initialize_bobyqa_mod_MOD_initxf: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___initialize_cobyla_mod_MOD_initfilt: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___initialize_cobyla_mod_MOD_initxfc: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___initialize_lincoa_mod_MOD_inith: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___initialize_lincoa_mod_MOD_initxf: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___initialize_newuoa_mod_MOD_inith: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___initialize_newuoa_mod_MOD_initq: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___initialize_newuoa_mod_MOD_initxf: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___initialize_uobyqa_mod_MOD_initl: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___initialize_uobyqa_mod_MOD_initq: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___initialize_uobyqa_mod_MOD_initxf: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___message_mod_MOD_fmsg: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___message_mod_MOD_cpenmsg: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___message_mod_MOD_rhomsg: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___message_mod_MOD_retmsg: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_setij: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_calvlag_qint: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_errh: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_omega_inprod: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_omega_mul: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_calbeta: register 77 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_calvlag_lfqint: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_calden: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_omega_col: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_updateh: register 79 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_hess_mul: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_quadinc_ghv: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_quadinc_d0: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_errquad: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_qrexc_rdiag: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_qradd_rfull: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___powalg_mod_MOD_qradd_rdiag: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___ratio_mod_MOD_redrat: register 22 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___rescue_mod_MOD_rescue: registers 76 and 77 not saved contiguously in frame
ld: warning: could not create compact unwind for ___selectx_mod_MOD_isbetter00.isra.0: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___selectx_mod_MOD_isbetter01: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___selectx_mod_MOD_isbetter10: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___selectx_mod_MOD_selectx: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___selectx_mod_MOD_savefilt: register 75 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___shiftbase_mod_MOD_shiftbase_qint: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___shiftbase_mod_MOD_shiftbase_lfqint: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___trustregion_bobyqa_mod_MOD_trsbox: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___trustregion_cobyla_mod_MOD_trstlp_sub.constprop.0: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___trustregion_cobyla_mod_MOD_trstlp: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___trustregion_lincoa_mod_MOD_trstep: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___trustregion_newuoa_mod_MOD_circle_fun_trsapp: registers 21 and 22 not saved contiguously in frame
ld: warning: could not create compact unwind for ___trustregion_newuoa_mod_MOD_trsapp: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___trustregion_uobyqa_mod_MOD_trstep: registers 78 and 79 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_bobyqa_mod_MOD_tryqalt: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_bobyqa_mod_MOD_updateq: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___update_bobyqa_mod_MOD_updatexf: register 26 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___update_bobyqa_mod_MOD_updateh: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_cobyla_mod_MOD_findpole: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_cobyla_mod_MOD_updatepole: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_cobyla_mod_MOD_updatexfc: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_lincoa_mod_MOD_updateres: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_lincoa_mod_MOD_tryqalt: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_lincoa_mod_MOD_updateq: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___update_lincoa_mod_MOD_updatexf: register 26 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___update_newuoa_mod_MOD_tryqalt: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___update_newuoa_mod_MOD_updateq: register 73 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___update_newuoa_mod_MOD_updatexf: register 26 saved somewhere other than in frame
ld: warning: could not create compact unwind for ___update_uobyqa_mod_MOD_update: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___xinbd_mod_MOD_xinbd: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___fprint_mod_MOD_fprint: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___getact_mod_MOD_delact.constprop.0.isra.0: registers 27 and 28 not saved contiguously in frame
ld: warning: could not create compact unwind for ___getact_mod_MOD_getact: registers 76 and 77 not saved contiguously in frame
ld: warning: could not create compact unwind for ___univar_mod_MOD_interval_max: registers 74 and 75 not saved contiguously in frame
ld: warning: could not create compact unwind for ___univar_mod_MOD_circle_maxabs: registers 72 and 73 not saved contiguously in frame
ld: warning: could not create compact unwind for ___univar_mod_MOD_circle_min: registers 72 and 73 not saved contiguously in frame

I'm not sure it's safe to just silence those with a linker flag (-Wl,-no_compact_unwind) before understanding what is happening there. Any ideas based on the code objects identified in the warning messages?

Also, I checked the binary size, _prima.cpython-312-darwin.so is 1.2 MB in release mode, while _cobyla.cpython-312-darwin.so was only 107 kb. That is quite a steep increase, and most of it comes from building the other solvers that we don't use. Can those be left out?

@@ -30,7 +30,7 @@ jobs:
- uses: actions/[email protected]
with:
fetch-depth: 0 # previous commits used in tools/lint.py
submodules: recursive
submodules: false
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this going to lead to failures showing up locally instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. Do you think the tools should instead be modified to check for submodule paths? In the case of lint.py it could easily generate a lot of errors against another repo that either doesn't have linting or allows a larger line length, and in both cases it's not something that SciPy can necessarily do anything about. Pretty much the same for unicode-check and check_test_name. I can do that in a separate PR, at which point the value of this variable would be moot and we could either change it back or leave it as is.

Copy link
Member

Choose a reason for hiding this comment

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

What was the failure that appeared in CI? I think we typically don't get local failures from submodules due to the --diff-against option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue happened because prima includes pybind11 as a submodule, and their test_builtin_casters fails the unicode-check (and they are indeed checking some unicode conversions) as well as check_test_name. Neither of those have the —diff-against option. Should I make a separate PR to refactor the submodule logic I added to check_installation so that it can be used in unicode-check and check_test_names? (And also can I please rename unicode-check to check_unicode so that it’s consistent with the other checks?)

.gitmodules Outdated
[submodule "scipy/_lib/prima"]
path = scipy/_lib/prima
url = https://www.github.com/nbelakovski/prima
branch = fixes
Copy link
Member

Choose a reason for hiding this comment

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

This is still pointing to a temporary place. I think before merging we'll create a scipy/prima repo to point at, which is an exact mirror of the upstream repo.

@@ -0,0 +1,101 @@
libquadmath_dep = cc.find_library('quadmath', required: false)
Copy link
Member

Choose a reason for hiding this comment

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

Recent discussion around libquadmath licensing: numpy/numpy#27080

MacPython/openblas-libs#178 is how to get rid of the snfprint symbol, that may be worth doing in PRIMA upstream.

Perhaps not essential if we're going to end up (a) including libquadmath anyway due to other Fortran code, and (b) translating this code to another language. I'd need to look at it more carefully.

Copy link
Member

Choose a reason for hiding this comment

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

Either way I wouldn't expect it to be necessary to explicitly detect libquadmath and list it as a dependency - if you're not using it directly, it should only be a transitive dependency from gfortran, and auditwheel will take care of including it as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was some discussion about this earlier: #20964 (comment)

Basically, on Windows the compilation was failing because it complained about not finding the libquadmath symbols, which led to calling out the dependency, but on macOS it complained that it couldn't find libquadmath, but compiled fine without it, hence setting required to false in order to make both Windows and macOS happy. As you can see from the discussion I linked numpy solved this issue similarly

I will explore the code in that PR you linked and report back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s not obvious to me how to apply this code to PRIMA. A search for ‘quadmath’ in that repo leads to 0 results, either in code or issues or PRs. Could this be a meson issue, or as you mentioned in the licensing discussion you linked, a gfortran issue? In any case, I don’t think it’s worth spending a lot of cycles on this given the next issue, unless there’s a license issue here that needs to be addressed.

There remains more work to do since PRIMA COBYLA can handle linear
constraints and can also handle a number of additional options.

To start with we should make sure the compilation works on all
supported platforms and then we will revisit adding linear constraints
(which will require some work on documentation, whereas so far none of
the changes made should affect documentation at all, except perhaps for
directly handling bounds.)

TST: I should note that I made significant changes to
test_minimize_constraint_violation because the new COBYLA implementation
performed better than the old one and broke the test, and also because the
test was very confusing and made unnecessary use of random numbers.
DOC: Documentation was also updated to reflect this.
Lint check was failing because of code in submodules. The lint check
should only be concerned with scipy code, as other modules may have
their own linting standard which may or may not overlap with scipy and
which may even conflict.
- Add fpp flags for intel compilation
- Modify tests to work with 32-bit
@nbelakovski nbelakovski force-pushed the prima branch 2 times, most recently from ae42e1a to 28aa2ee Compare November 20, 2024 19:10
@nbelakovski
Copy link
Contributor Author

nbelakovski commented Nov 20, 2024

Hi all,

Sorry to drop away for a while. As happens from time to time life gets in the way but I’m back now and ready to drive this to completion.

@rgommers raised 6 issues, I’ll address them here and also in the relevant comment threads where appropriate:

  1. CirrusCI aarch64 job is failing. ENH: optimize: integrate the PRIMA library for COBYLA #20964 (comment)

It’s failing because it’s unable to clone the prima_development submodule of the prima repo. I’m not sure what to say on this one. The dev repo is public: https://github.com/libprima/prima_development/. This sounds like an issue with the setup of this particular CI job, is there someone who can help debug this? We don’t need the prima_development submodule, it’s just getting pulled in recursively.

  1. The linker emits a warning about “could not create compact unwind for …” ENH: optimize: integrate the PRIMA library for COBYLA #20964 (review)

I was able to find this: https://www.scivision.dev/cmake-compact-unwind-ld-warning/

Basically the issue happens when using AppleClang with gfortran, and in theory it impacts exception handling, but I tested raising exceptions in the objective function, in the constraint function, and also within the C++ wrapper and they all function as expected (i.e. you get a nice traceback instead of just a crash or exit). I also tested linking with g++ instead of c++ and the warning went away, confirming the advice from the above link. However, it appears that some tests of exception handling are failing, despite the fact that they are unrelated to prima/cobyla (test_isotonic_regression and test_distance). I am continuing to investigate this.

  1. The binary size is 1MB compared to the previous code as 107kB, can we avoid compiling the other solvers to get this down? ENH: optimize: integrate the PRIMA library for COBYLA #20964 (review)

Yes, but this still resulted in a binary of 840k. I’ve made the changes and pushed them.

  1. Linting: to recurse or not to recurse? ENH: optimize: integrate the PRIMA library for COBYLA #20964 (comment)

The issue happened because prima includes pybind11 as a submodule, and their test_builtin_casters fails the unicode-check (and they are indeed checking some unicode conversions) as well as check_test_name. Neither of those have the —diff-against option. Should I make a separate PR to refactor the submodule logic I added to check_installation so that it can be used in unicode-check and check_test_names? (And also can I please rename unicode-check to check_unicode so that it’s consistent with the other checks?)

  1. libquadmath ENH: optimize: integrate the PRIMA library for COBYLA #20964 (comment)

It’s not obvious to me how to apply this code to PRIMA. A search for ‘quadmath’ in that repo leads to 0 results, either in code or issues or PRs. Could this be a meson issue, or as you mentioned in the licensing discussion you linked, a gfortran issue? In any case, I don’t think it’s worth spending a lot of cycles on this given the next issue, unless there’s a license issue here that needs to be addressed.

  1. Temporary place ENH: optimize: integrate the PRIMA library for COBYLA #20964 (comment)

Unfortunately @zaikunzhang no longer appears to be active in PRIMA. He makes some small commits every 2 months to prevent CI from expiring, but he’s not commenting on issues or PRs, not even a small PR of mine from June to fix a couple build issues discovered while working on this integration with scipy. I’ve made a handful of changes to deal with various issues brought up in this integration so we have to point to my copy. You can mirror it in scipy if you like, but we can’t mirror the main repo. I think this also means that we shouldn’t include the other solvers via the prima python package as optional dependencies and instead work towards rewriting the algorithm in Python. I’ve made some steps there and I think I can contribute that in the short/medium term, but I think we should get this PR across the finish line since it’s extremely close.

@nbelakovski
Copy link
Contributor Author

An update on the no_compact_unwind situation: it turns out this flag does not simply suppress a warning message, it suppresses the generation of compact unwinds. This leads to issues when raising exceptions from C++, apparently even in libraries that weren't compiled with this flag (as the failure of the TestIsoTonicRegression shows)?? It's a bit confusing because despite the warnings, C++ exceptions work. When compiling with no_compact_unwind the warnings go away, but now C++ exceptions don't work (Python doesn't catch them and they abort the program), so I think we're stuck with those error messages, assuming that's acceptable.

Incorporate changes to PRIMA that allow us to not compile the other
solvers. This reduces the binary from about 1.2MB to 840kB.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement Fortran Items related to the internal Fortran code base Meson Items related to the introduction of Meson as the new build system for SciPy scipy._lib scipy.optimize
Projects
None yet
8 participants