Remove tropter
#3988
Remove tropter
#3988
Conversation
|
Hey @aymanhab, I just noticed you started working on this. I think I misunderstood your message from last week; I was under the impression that you were going to disable the test suite for tropter when building the conda packages, and not remove them directly here. I was hoping to address all the tropter changes at once (i.e., remove the tropter libraries and the tests together). I'm a bit hesitant to gut out the tests while the tropter solver still exists as an option. Is there a way to build the conda packages without tropter for now (i.e., by setting the |
|
No problem @nickbianco I actually have a conda package without tropter using the cmake option already (https://anaconda.org/opensim_admin/opensim/files) |
remove tropter dependency in ci
|
@nickbianco I went ahead and removed references to tropter from the build system and ci, and the last remaining piece is that some python tests in test_moco.py are failing because they were using tropter without checking if available When you have a chance let me know if I should remove some/all of these tests that use tropter or convert to casadi Thank you |
|
@aymanhab it would be best to keep those tests and convert them all to use Were you planning on removing the tropter libraries as part of this PR too? |
|
@nickbianco My plan was to make the build/install/test all work first and removed from the make system on this branch and then remove the folders (in case something comes up that force a backtrack) but it can go in any order. Do you expect any results to change when swapping the solvers? |
|
That sounds like a good plan. Theoretically, the tests should pass with either solver, but it's possible that differences in each implementation could lead to different numerical results. I'm happy to debug any such cases that arise. We will also need to eventually remove Tropter from all of the tests under the Moco test folder. |
|
@nickbianco I'll pass this on to you to do the internal cleanup of the Moco tests. The python test cases pass on linux but not on windows or osx though the failures are for reasons unrelated to this PR as far as I can tell. Just let me know if you want me to investigate these failures. Thank you. |
|
Sure @aymanhab. I'll pick this up starting next week. |
|
Thanks @nickbianco 👍 |
nickbianco
changed the title
tropter
|
@aymanhab, the scripting tests were failing because the Ipopt libs were no longer being copied into the install directory. I've fixed this for Mac; now I'm trying to figure out where the copy was previously happening for Windows. I also preemptively updated the CMake files to carry over the rpaths needed for CasADi on Linux (which somehow ended up in tropter's directory). I think these rpaths were mainly needed for the conda packages? Maybe you could take a look at the latest commits to see if it makes sense to you. |
|
Line 796 in 2746e8f This copies casadi, as to ipopt and other dependencies they're copied later at the same file under ipopt block |
|
@nickbianco Conda does its own rpath setting since it has a different layout so don't worry about it and let's get the installation to be self contained regardless of conda |
|
Sounds good @aymanhab. I think the issue on Windows has to do with one of the Ipopt dependencies being missing in the install directory, but I will continue to investigate. |
|
@aymanhab, I finished cleaning up all references to Tropter in the documentation. The last thing for you to look over are the changes I made releated to RPaths, i.e., |
There was a problem hiding this comment.
@nickbianco The only material change I saw was in a message, so
Reviewable status: 0 of 176 files reviewed, all discussions resolved
|
@aymanhab, just to clarify, Also, should we get another reviewer? This is a pretty big change. |
|
@nickbianco It wouldn't hurt to get another pair of eyes though the number of files is intimidating. Primarily I'd clarify that this PR is gutting out Tropter as a Moco solver from the codebase/build-system so no new functionality is added/tested. For the make files, I glanced over them and looked like they were moved rather than rewritten, and they looked fine. Though taking a second look now I realize that the names of the libraries have changed a while ago (on mac/linux there's no coinmetis, only metis) |
|
@carmichaelong, tagging you for a review here. It is a huge PR, but the vast majority of changes are removed files and code. The changes to the build system are the main things that should be removed. @adamkewley also tagging you here as an FYI. Just in case you wanted to take a look at (and maybe review, if you're feeling generous) the changes to the build system and make a note of anything that might affect OpenSim Creator downstream. Hopefully, the removal of the dependencies should make things simpler on your end (although I'm not sure you build those dependencies in the first place). |
| if(UNIX) | ||
| pkg_check_modules(IPOPT REQUIRED ipopt IMPORTED_TARGET) | ||
|
|
||
| if(OPENSIM_COPY_DEPENDENCIES AND APPLE) |
There was a problem hiding this comment.
I think, of the changes, these additions are the ones that make me go hmmmmm.
It looks like you're trying to tell cmake to add those libraries as runtime libraries to the installation set. Fair enough, but cmake has functions specifically for doing this, such as install(TARGETS libOsimMoco RUNTIME_DEPENDENCIES).
If cmake's dependency resolution mechanisms don't work, it's usually a sign that your library targets (i.e. the things passed to target_link_libraries) aren't declaring which files are dependent on which. Do as you wish, but the way this is written right now smells fishy (e.g. it won't work if the Linux target architecture is changed from x86_64, or if it's ran on a distribution of Linux that has a different sys library directory layout).
|
Other than the single comment, which you can take or leave (because I'm not planning on compiling moco: its dependency tree is still too big relative to the payoff in OSC), looks good - I'm generally a fan of dropping dependencies 😉 |
|
@adamkewley thanks for the quick turnaround. Indeed our cmake use needs to be overhauled, the copying of dependencies is a side effect of having a two stage build process where the opensim-core build system deals with artifacts/files rather than targets that have other dependencies baked into their build. This though is beyond the scope of this PR. One hurdle is that the Moco stack is deep so the dependency propagation has to be baked into other large projects that we use (e.g ipopt and its dependencies etc.) but feel free to open an issue or let me know and I'll open one so we can address in due time. |
There was a problem hiding this comment.
agreed that while at some point refactoring our cmake files would be good, this PR is looking good as is. i left one comment about a test being removed all together (rather than keeping it with casadi) but i'm not too worried, just a double check
Reviewed 5 of 9 files at r1, 102 of 103 files at r3, 60 of 63 files at r4, 8 of 8 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aymanhab)
OpenSim/Moco/Test/testMocoInterface.cpp line 1255 at r6 (raw file):
} TEMPLATE_TEST_CASE("Guess time-stepping", "[tropter]",
does this test need to be kept with the casadi solver? my gut says that all these pieces are likely tested elsewhere, so i'd lean not, but just dou9ble checking
There was a problem hiding this comment.
Thanks @carmichaelong and @adamkewley! I've added the time-stepping test back in.
Reviewable status: 175 of 176 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @carmichaelong)
OpenSim/Moco/Test/testMocoInterface.cpp line 1255 at r6 (raw file):
Previously, carmichaelong wrote…
does this test need to be kept with the casadi solver? my gut says that all these pieces are likely tested elsewhere, so i'd lean not, but just dou9ble checking
This is a good test to include, I've added it back in.
There was a problem hiding this comment.
Reviewable status: 175 of 176 files reviewed, 1 unresolved discussion (waiting on @aymanhab)
There was a problem hiding this comment.
Reviewable status: 175 of 176 files reviewed, 1 unresolved discussion (waiting on @carmichaelong)
Fixes issue opensim-org/opensim-gui#1533
Brief summary of changes
Remove the
tropterlibrary,MocoTropterSolver, and all references to Tropter in the tests, documentation, examples, etc. As a result, the following dependencies has been removed:eigen,adolc, andcolpack.Tropter is only very minimally used compared to CasADi in Moco, so we've decided that the burden to support Tropter and its dependencies is not worth our development effort going forward. Additionally, these changes will open the door to more efficient implementation of Moco's direct collocation libraries that could leverage CasADi's automatic differentiation tools.
Testing I've completed
Ran test suite.
Looking for feedback on...
Currently, the Python tests are failing on Windows CI. These test pass locally, it is unclear why they fail in the Windows GitHub Actions runners.
CHANGELOG.md (choose one)
This change is