Upgrade casadi/ipopt stack #3693

aymanhab · 2024-01-29T19:16:41Z

Fixes issue #0
Initially the plan was to upgrade to a more stable/tagged ipopt (as the branch name suggests), however it appears latest casadi release is built against custom ipopt repo/version based on 3.14.11.mod because of that I'm leaning to remove the separate ipopt dependency and instead have casadi download/install its own dependencies.

Brief summary of changes

Upgrade to more recent casadi/ipopt stack

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

no need to update because...
updated.

This change is

… to configure as is

aymanhab · 2024-01-29T19:18:54Z

@nickbianco Would like to hear your thoughts on the description of what this PR wants to do and if you foresee problems with this approach.

nickbianco · 2024-01-29T20:07:15Z

@aymanhab, that sounds reasonable to me, assuming that the version of Ipopt plays nice with all non-CasADi Ipopt usage in OpenSim. Is this version of Ipopt compatible with Mac Silicon?

…ts own custom versions of dependencies

aymanhab · 2024-02-06T19:05:34Z

Considering that casadi uses a custom ipopt internally, using the latest tagged/public version seems more problematic than needed, so will stick with the version maintained by Joris at https://github.com/jgillis/Ipopt-1.git to make sure latest casadi works then test if tropter can use it.

… TROPTER options

aymanhab · 2024-02-08T19:04:09Z

Building latest casadi with its own ipopt on linux succeeds but fails one moco related test after fixing 2 compilation errors. Refactoring to use the same ipopt for both casdai and tropter before asking for review. Windows has a totally different pathway in CMake that fails to build out-of-the-box. Investigating

Remove dependency of casadi on ipopt

tropter off on *nix

aymanhab · 2024-02-14T16:54:20Z

@carmichaelong I have a conda package on linux (opensim-moco 4.5.1 opensim_admin) including the updated casadi (no tropter) here, any idea how to test this? Thank you
https://anaconda.org/opensim_admin/opensim-moco/files

nickbianco · 2024-02-14T17:27:10Z

@aymanhab @carmichaelong MocoCasADiSolver is the default solver for pretty much all the Moco examples, if you wanted to run those as a starting point.

…flow, build succeeds but install/run fails

…m-core into upgrade_ipopt

aymanhab · 2024-02-28T16:31:34Z

@nickbianco or @carmichaelong Would it be possible to take a look at the changes to the two header files in this PR and the Moco test failures to let me know if these are signs of bigger problems? Also I made a conda package on linux that I would like to test on jupyter. If you have or can point to a short snippet/example that exercises casadi I'd appreciate it.

…pping_test_fix Allow empty controls table when creating time-stepping guesses

nickbianco · 2024-05-27T00:05:36Z

Merged in the fix for the testMocoInterface test. I'll look into fixes for the other test failures.

aymanhab · 2024-05-27T01:35:09Z

Awesome, thanks @nickbianco

…_fix Update `testMocoTrack.cpp`

skip test case on mac

aymanhab · 2024-05-29T00:06:20Z

All tests now pass on all platforms thanks much @nickbianco for the fixes and the review.

nickbianco

No problem Ayman! One quick comment about disabling testMocoImplicit (we only need to disable it for Mac).

I can leave a full review for this PR whenever it's ready.

nickbianco · 2024-05-29T03:39:11Z

.github/workflows/continuous_integration.yml

@@ -597,7 +598,7 @@ jobs:
    - name: Test opensim-core
      run: |
        cd $GITHUB_WORKSPACE/../build
-        ctest --parallel 4 --output-on-failure
+        ctest --parallel 4 --output-on-failure -E testMocoImplicit


Rather than exclude the test manually here, you can add a flag to the Catch2 test definition to only exclude the test on Mac. See CONTRIBUTING.md for details.

nickbianco · 2024-05-29T19:57:34Z

@aymanhab My bad: the Mac test failed again because the tagging logic only applies to tests defined using OpenSimAddTests. If you add that logic to MocoAddTest here it should work as expected.

nickbianco · 2024-05-29T19:58:27Z

We should eventually have Moco tests use OpenSimAddTests instead of the Moco-specific macro, but we can handle that in a future PR.

aymanhab · 2024-05-29T20:01:06Z

No worries, will do. Thanks @nickbianco

aymanhab · 2024-05-30T05:29:36Z

Ready for review @nickbianco

nickbianco

Thanks @aymanhab for tackling these changes! I know it was a bear to get through.

I left a bunch of mostly minor comments about commented out lines and whether we should keep them. There were some modifications to the CI script that I wasn't sure about either. Finally, I had quick question about the ordering of the dependencies during the build process, specifically requiring Ipopt to come before CasADi and how the build system is handling that.

Reviewed 17 of 18 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @aymanhab)

.github/workflows/continuous_integration.yml line 80 at r3 (raw file):

        # Set the CXXFLAGS environment variable to turn warnings into errors.
        # Skip timing test section included by default.
        cmake -E env CXXFLAGS="/WX -DSKIP_TIMING" cmake $env:GITHUB_WORKSPACE -LAH -G"Visual Studio 16 2019" -A x64 -DCMAKE_INSTALL_PREFIX=~/opensim-core-install -DOPENSIM_DEPENDENCIES_DIR=~/opensim_dependencies_install -DOPENSIM_C3D_PARSER=ezc3d -DBUILD_PYTHON_WRAPPING=on -DBUILD_JAVA_WRAPPING=on -DOPENSIM_WITH_TROPTER=off -DPython3_ROOT_DIR=C:\hostedtoolcache\windows\Python\3.8.10\x64

Revert this change? Tropter builds are disabled throughout the CI script, did you mean to revert those too?

.github/workflows/continuous_integration.yml line 201 at r3 (raw file):

    - name: Build dependencies
      #if: steps.cache-dependencies.outputs.cache-hit != 'true'

Re-enable caching?

.github/workflows/continuous_integration.yml line 309 at r3 (raw file):

    - name: Build dependencies
      # if: steps.cache-dependencies.outputs.cache-hit != 'true'

Re-enable caching?

.github/workflows/continuous_integration.yml line 933 at r3 (raw file):

        DEP_CMAKE_ARGS+=(-DCMAKE_BUILD_TYPE=Release)
        DEP_CMAKE_ARGS+=(-DSUPERBUILD_ezc3d=ON)
        DEP_CMAKE_ARGS+=(-DOPENSIM_WITH_TROPTER=ON)

Disable Tropter build here?

cmake/FindIPOPT.cmake line 1 at r3 (raw file):

find_package(Ipopt CONFIG)

nit: Leave a note at the top of the file that this was adapted from CasADi's build scripts.

cmake/FindIPOPT.cmake line 37 at r3 (raw file):

       list(APPEND IPOPT_INCLUDE_DIRS "${CMAKE_SOURCE_DIR}/external_packages/ipopt/3.10")
       set(WITH_IPOPT_CALLBACK TRUE)
       # message(STATUS "Detected an IPOPT configuration without development headers. Build will proceed, but without callback functionality. To enable it, see https://github.com/casadi/casadi/wiki/enableIpoptCallback")

Remove?

cmake/OpenSimMocoInstallMacDependencyLibraries.cmake.in line 65 at r3 (raw file):

    install_name_tool_add_rpath(casadi.3.7)
    install_name_tool_delete_rpath(casadi.3.7 "@CASADI_LIBDIR@")
    #install_name_tool_delete_rpath(casadi_nlpsol_ipopt.3.7 "${libquadmath_dir}")

Should these commented out lines be removed? Or are they being kept for reference?

dependencies/CMakeLists.txt line 43 at r3 (raw file):

endfunction()

# CMake doesn't clear prefix directories when user changes it.

Remove trailing whitespace.

dependencies/CMakeLists.txt line 76 at r3 (raw file):

#   GIT_TAG    -- (Required) git tag to checkout before commencing build.
#   DEPENDS    -- (Optional) Other projects this project depends on.
#   CMAKE_ARGS -- (Optional) A CMake list of arguments to be passed to CMake

Remove trailing whitespace.

dependencies/CMakeLists.txt line 267 at r3 (raw file):

    if(SUPERBUILD_ipopt)

minor: One thing I noticed when building the dependencies locally with these changes: the CasADi build will fail if the Ipopt build hasn't completed. I was building locally using the "all build" target, and maybe that's just a limitation of that approach. How do the build scripts/CI handle the ordering of dependencies.

dependencies/CMakeLists.txt line 276 at r3 (raw file):

            "${CMAKE_BINARY_DIR}/ipopt-prefix/src/ipopt"
            "${CMAKE_INSTALL_PREFIX}/ipopt")
        ExternalProject_Add(ipopt

Remove trailing whitespace.

dependencies/CMakeLists.txt line 359 at r3 (raw file):

            INSTALL_DIR       "${CMAKE_INSTALL_PREFIX}/mumps"
            INSTALL_COMMAND ${CMAKE_MAKE_PROGRAM} install)

Remove extra whitespace.

dependencies/CMakeLists.txt line 364 at r3 (raw file):

            URL https://github.com/coin-or/Ipopt/archive/refs/tags/releases/3.14.16.zip
            INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/ipopt"
            #PATCH_COMMAND cd <SOURCE_DIR>/ && patch < ${CMAKE_SOURCE_DIR}/joris_fixipopt.patch

If the patch file from Joris is not being used, should it be removed?

dependencies/joris_fixipopt.patch line 1 at r3 (raw file):

--- a/src/Algorithm/LinearSolvers/IpTSymLinearSolver.cpp

Remove this patch if unused?

Vendors/tropter/cmake/CMakeLists.txt line 14 at r3 (raw file):

        OUTPUT_VARIABLE libgcc_name
        OUTPUT_STRIP_TRAILING_WHITESPACE)

Remove whitespace.

Vendors/tropter/cmake/CMakeLists.txt line 30 at r3 (raw file):

            # ${gfortran}
            # ${quadmath}
            # ${gcc_libdir}/${libgcc_name}

Remove?

Vendors/tropter/cmake/CMakeLists.txt line 77 at r3 (raw file):

            ${gfortran}
            ${quadmath}
            #${gcc_libdir}/libgcc_s.so

Remove?

Vendors/tropter/cmake/tropter_install_mac_dependency_libraries.cmake.in line 81 at r3 (raw file):

install_name_tool_change(tropter coinmumps.3 "@IPOPT_LIBDIR@")
install_name_tool_change(tropter coinmetis.1 "@IPOPT_LIBDIR@")
# install_name_tool_change_gfortran(tropter)

Remove commented lines in this file?

Vendors/tropter/tropter/CMakeLists.txt line 82 at r3 (raw file):

    target_link_libraries(tropter PRIVATE PkgConfig::IPOPT)
else()
    # message("Adding ipopt depndency in link line for tropter ${Ipopt_LIBRARIES}")

Suggestion:

    # message("Adding ipopt dependency in link line for tropter ${Ipopt_LIBRARIES}")

Vendors/tropter/tropter/CMakeLists.txt line 94 at r3 (raw file):

    # In CMake 3.13, we could use target_link_options().
    #message("LINK_FLAGS ${IPOPT_LDFLAGS}")
    #target_link_libraries(tropter PROPERTIES LINK_FLAGS ${IPOPT_LDFLAGS})

Remove?

aymanhab

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @nickbianco)

.github/workflows/continuous_integration.yml line 601 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Rather than exclude the test manually here, you can add a flag to the Catch2 test definition to only exclude the test on Mac. See CONTRIBUTING.md for details.

Done.

.github/workflows/continuous_integration.yml line 201 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Re-enable caching?

Caching doesn't appear to catch all the changes of dependencies so we have to disable it once to update the cache then enable it later. I may have been more cynical about it but at that point getting full fresh builds is top priority, optimizations later.

.github/workflows/continuous_integration.yml line 309 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Re-enable caching?

same comment

.github/workflows/continuous_integration.yml line 933 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Disable Tropter build here?

Sure 👍

disable tropter build for build without moco [skip ci]

Add reference to source of script [ci skip]

aymanhab

Reviewable status: 12 of 20 files reviewed, 21 unresolved discussions (waiting on @nickbianco)

cmake/FindIPOPT.cmake line 1 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

nit: Leave a note at the top of the file that this was adapted from CasADi's build scripts.

Agreed

cmake/FindIPOPT.cmake line 37 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove?

This was in the original file from casadi so I didn't want to introduce diffs unnecessarily

cmake/OpenSimMocoInstallMacDependencyLibraries.cmake.in line 65 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Should these commented out lines be removed? Or are they being kept for reference?

They were kept for reference but in retrospect we have too many of these legacy lines it would be cleaner to remove, so I'll remove them, we can use github to recover if issues come up with installation on macs.

dependencies/CMakeLists.txt line 43 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove trailing whitespace.

Done.

dependencies/CMakeLists.txt line 76 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove trailing whitespace.

Done.

dependencies/CMakeLists.txt line 267 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

minor: One thing I noticed when building the dependencies locally with these changes: the CasADi build will fail if the Ipopt build hasn't completed. I was building locally using the "all build" target, and maybe that's just a limitation of that approach. How do the build scripts/CI handle the ordering of dependencies.

Not sure what platform but make builds a dependency graph to make sure every target has its dependencies built first, so it's possible we may need to explicitly add dependency somewhere if not deducted. If you use multiple threads this may get exposed. Will keep an eye.

dependencies/CMakeLists.txt line 276 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove trailing whitespace.

Done.

dependencies/CMakeLists.txt line 359 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove extra whitespace.

Done.

dependencies/CMakeLists.txt line 364 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

If the patch file from Joris is not being used, should it be removed?

It's hard to tell if it's ever needed in moco, it ties us to a specific codebase/layout so I'd remove it

Vendors/tropter/cmake/CMakeLists.txt line 14 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove whitespace.

Done.

Vendors/tropter/cmake/CMakeLists.txt line 30 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove?

Done.

Vendors/tropter/cmake/CMakeLists.txt line 77 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove?

Done.

Vendors/tropter/cmake/tropter_install_mac_dependency_libraries.cmake.in line 81 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove commented lines in this file?

Done.

Vendors/tropter/tropter/CMakeLists.txt line 82 at r3 (raw file):

    target_link_libraries(tropter PRIVATE PkgConfig::IPOPT)
else()
    # message("Adding ipopt depndency in link line for tropter ${Ipopt_LIBRARIES}")

Done.

Vendors/tropter/tropter/CMakeLists.txt line 94 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove?

Done.

dependencies/joris_fixipopt.patch line 1 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove this patch if unused?

Done.

nickbianco

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aymanhab)

.github/workflows/continuous_integration.yml line 201 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Caching doesn't appear to catch all the changes of dependencies so we have to disable it once to update the cache then enable it later. I may have been more cynical about it but at that point getting full fresh builds is top priority, optimizations later.

👍

.github/workflows/continuous_integration.yml line 309 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

same comment

👍

cmake/FindIPOPT.cmake line 37 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

This was in the original file from casadi so I didn't want to introduce diffs unnecessarily

Sounds good!

dependencies/CMakeLists.txt line 267 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Not sure what platform but make builds a dependency graph to make sure every target has its dependencies built first, so it's possible we may need to explicitly add dependency somewhere if not deducted. If you use multiple threads this may get exposed. Will keep an eye.

Ah, I think building locally with multiple threads is the issue. Since the CI builds are fine, we can address this in a later PR if it becomes a wider problem.

aymanhab · 2024-05-30T22:08:48Z

Awesome thanks much for all your help and review @nickbianco 👍

aymanhab added 2 commits January 29, 2024 07:55

Download prebuilt ipopt 3.14.14 from coin-or on windows

6fb4001

Document compilers/runtime used to build windows binary, casadi fails…

c970fab

… to configure as is

Remove dependency of casadi on ipopt and have it download and build i…

844e285

…ts own custom versions of dependencies

aymanhab changed the title ~~[WIP} Upgrade ipopt~~ Upgrade casadi/ipopt stack Feb 6, 2024

Restore dependency of CASADI on IPOPT so we can build both CASADI and…

ccb21e9

… TROPTER options

aymanhab added 7 commits February 12, 2024 12:04

download and install metis separately

c35d429

use internal build of ipopt, mumps and metis for now and run tests

5c058d5

Update CMakeLists.txt

23357e1

Remove dependency of casadi on ipopt

Update continuous_integration.yml

1e7747f

Disable independent ipopt build stack as done internally by casadi

6e2c7b9

remove use of ipopt for dependencies

e9afd49

Update continuous_integration.yml

9d208c4

tropter off on *nix

aymanhab and others added 11 commits February 15, 2024 00:14

update casadi flags to build ipopt

637b73e

Add patch on top of a tagged ipopt

9e16912

Pass flags to Metis to build correctly

d08721d

Separate mumps and metis, then add dependencies, restore TROPTER work…

2003e9c

…flow, build succeeds but install/run fails

Remove debugging line in cmake

bff6bff

Update CMakeLists.txt

4c4fd2c

Merge branch 'upgrade_ipopt' of https://github.com/opensim-org/opensi…

83faf46

…m-core into upgrade_ipopt

Specify mumps for ipopt during configure

baf494f

configure ipopt with mumps

0603ecf

Merge branch 'upgrade_ipopt' of https://github.com/opensim-org/opensi…

f6de43c

…m-core into upgrade_ipopt

Build on osx arm64

de47133

nickbianco added 2 commits May 26, 2024 17:02

Use boolean over int comparisons

5b8f6a4

Merge pull request #3791 from opensim-org/upgrade_ipopt_guess_timeste…

67276e3

…pping_test_fix Allow empty controls table when creating time-stepping guesses

Update testMocoTrack standard solution file

74a3f8c

nickbianco mentioned this pull request May 27, 2024

Update testMocoTrack.cpp #3792

Merged

Make the tolerances a bit tighter

e3441d0

nickbianco and others added 3 commits May 26, 2024 18:59

Adjust testMocoTrack to test state tracking instead of controls

de9c5de

Merge pull request #3792 from opensim-org/upgrade_ipopt_testMocoTrack…

ef88f12

…_fix Update `testMocoTrack.cpp`

Update continuous_integration.yml

22007b3

skip test case on mac

nickbianco requested changes May 29, 2024

View reviewed changes

Move skipping testMocoImplicit from ci script to test file

c07d6a1

aymanhab added 2 commits May 29, 2024 14:27

Duplicate cmake code to run Moco tests per platform specification

ab01384

revert reset list of args introduced by last commit

d1c8089

nickbianco requested changes May 30, 2024

View reviewed changes

aymanhab requested a review from nickbianco May 30, 2024 17:47

aymanhab commented May 30, 2024

View reviewed changes

aymanhab added 3 commits May 30, 2024 10:50

Update continuous_integration.yml

7ce9aac

disable tropter build for build without moco [skip ci]

Update FindIPOPT.cmake

7ba4c28

Add reference to source of script [ci skip]

Remove commented out lines in CMake files and formatting per code review

f770c0e

aymanhab commented May 30, 2024

View reviewed changes

nickbianco approved these changes May 30, 2024

View reviewed changes

aymanhab merged commit 8fbf616 into main May 30, 2024
11 of 12 checks passed

aymanhab deleted the upgrade_ipopt branch May 30, 2024 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade casadi/ipopt stack #3693

Upgrade casadi/ipopt stack #3693

aymanhab commented Jan 29, 2024 •

edited

aymanhab commented Jan 29, 2024

nickbianco commented Jan 29, 2024

aymanhab commented Feb 6, 2024

aymanhab commented Feb 8, 2024

aymanhab commented Feb 14, 2024

nickbianco commented Feb 14, 2024

aymanhab commented Feb 28, 2024

nickbianco commented May 27, 2024

aymanhab commented May 27, 2024

aymanhab commented May 29, 2024

nickbianco left a comment

nickbianco May 29, 2024

nickbianco commented May 29, 2024

nickbianco commented May 29, 2024

aymanhab commented May 29, 2024

aymanhab commented May 30, 2024

nickbianco left a comment

aymanhab left a comment

aymanhab left a comment

nickbianco left a comment

aymanhab commented May 30, 2024

Upgrade casadi/ipopt stack #3693

Upgrade casadi/ipopt stack #3693

Conversation

aymanhab commented Jan 29, 2024 • edited

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

aymanhab commented Jan 29, 2024

nickbianco commented Jan 29, 2024

aymanhab commented Feb 6, 2024

aymanhab commented Feb 8, 2024

aymanhab commented Feb 14, 2024

nickbianco commented Feb 14, 2024

aymanhab commented Feb 28, 2024

nickbianco commented May 27, 2024

aymanhab commented May 27, 2024

aymanhab commented May 29, 2024

nickbianco left a comment

Choose a reason for hiding this comment

nickbianco May 29, 2024

Choose a reason for hiding this comment

nickbianco commented May 29, 2024

nickbianco commented May 29, 2024

aymanhab commented May 29, 2024

aymanhab commented May 30, 2024

nickbianco left a comment

Choose a reason for hiding this comment

aymanhab left a comment

Choose a reason for hiding this comment

aymanhab left a comment

Choose a reason for hiding this comment

nickbianco left a comment

Choose a reason for hiding this comment

aymanhab commented May 30, 2024

aymanhab commented Jan 29, 2024 •

edited