search

borglab / gtsam

GTSAM is a library of C++ classes that implement smoothing and mapping (SAM) in robotics and vision, using factor graphs and Bayes networks as the underlying computing paradigm rather than sparse matrices.
http://gtsam.org
Other
2.54k stars 753 forks source link

[Feature Request] Allow use of system libraries for 3rdparty codebases #292

Open acxz opened 4 years ago

acxz commented 4 years ago

Feature

While having 3rdparty codebases inside of GTSAM's codebase is very useful, such as keeping track of dependencies and building off of one version, it is also useful to allow for the use of system libraries, so that users can have consistent versions across their own projects and computer and that upstream can be followed readily for bug fixes/new features.

List of 3rd part modules with git* links that need to be accounted for:

Motivation

All 3rdparty libraries in GTSAM are included in most distributions package managers, (def in Debian/Ubuntu and Arch, most likely in Fedora as well). In some cases having the GTSAM 3rdparty libraries causes conflicts. For example the ArchLinux package has been patched so that GTSAM's metis does not conflict with the actual metis library. (https://github.com/borglab/gtsam/issues/220) At least having the option to use system libraries will help alleviate this and allow more freedom on the user's end. This also decreases the GTSAM installed size as well.

Pitch

Add flags and relevant linking code in the CMake procedure to allow for the use of system level libraries over 3rdparty libraries. Similar to the how users can use system installed eigen or geographiclib.

Alternatives

N/A

Additional context

Most distros like to see codebases 'devendored' so as to decrease the amount of code reuse, linking errors, etc. It is also the reason why distro like to dynamically link against libraries instead of statically linking everything.

ruffsl commented 4 years ago

This may be a good step forward for packaging and distribution. I was about to open the ticket requesting about distributing binaries via Debian packages. There's a number of slam frameworks within the ROS community that never quite get released into binary distributions of release distros for easy end user installment given their dependencies never get released/shipped.

For example, packages like SLAM-Toolbox can use multiple different backends, like Ceres, G2O, SPA, and GTSAM, but end up deprecating backends that aren't released as they can't be used and thus not as useful to maintain.

https://github.com/SteveMacenski/slam_toolbox

There are also some impressive VIO SLAM packages I'd like to see disrupt the robotic software stack, but the usually get stunted to user-exercises in source builds before they ever get community traction. I'd like to see Kimera-VIO make it into a ROS release, but that will require a number of upstream dependencies to be released as well, including GTSAM.

https://github.com/MIT-SPARK/Kimera-VIO

I see there's a debian folder here that was last touched a while ago. In the interim of getting a full-blown Debian package released upstream, merely providing Debian packages within the ROS repository via bloom and the ROS build farm maybe a simpler/quicker method of distributing debians for downstream libraries.

https://index.ros.org/doc/ros2/Tutorials/Releasing-a-ROS-2-package-with-bloom

acxz commented 4 years ago

requesting about distributing binaries via Debian packages

GTSAM does have a Ubuntu PPA, See: https://gtsam.org/get_started/

like to see Kimera-VIO make it into a ROS release

Some distributions do package it already, such as Arch Linux, in the end it depends on the userbase/community and how willing they are to package it upstream, See: https://aur.archlinux.org/packages/kimera-vio

See the following related issue: https://github.com/borglab/gtsam/issues/90

In the end it depends on the community that wants to maintain and package it. There is interest on Ubuntu so there is a Ubuntu package and similarly for Arch Linux, if someone is interested enough in using GTSAM in the ROS framework then it'll be a part of ROS.

ruffsl commented 4 years ago

GTSAM does have a Ubuntu PPA

PPAs are useful for building downstream libraries, but unfortunately not so much in releasing them.

Some distributions do package it already, such as Arch Linux

Nice! Although I couldn't find any users or companies using Arch in production for ROS applications

https://metrics.ros.org/index.html

It looks like most user stick with LTS OSs and architectures that have Tier 1 support:

https://www.ros.org/reps/rep-2000.html

In the end it depends on the community that wants to maintain and package it.

I'd argue is still take some cooperation with core maintainers, as it can be tricky for external parties to make a library's dependencies and build system hygienic enough for packaging. What current 3rdparty dependencies does GTSAM use use that could instead swapped to system installed libraries?

dellaert commented 4 years ago

I think Boost installed, and we have vendored most everything else.

acxz commented 4 years ago

What current 3rdparty dependencies does GTSAM use use that could instead swapped to system installed libraries?

From OP:

All 3rdparty libraries in GTSAM are included in most distributions package managers, (def in Debian/Ubuntu and Arch, most likely in Fedora as well).

ruffsl commented 4 years ago

@acxz lets get a little more specific, as some of your vendor-ed libs don't even list major versions. From a naive search, this is what I found in Ubuntu, though I'm not sure all are substitutable:

varunagrawal commented 4 years ago

While we're on this subject, it might also be a good idea to make all the 3rd party code as submodules so managing them via git from upstream is a piece of cake. Using submodules, we can easily pin various versions through the commit hash, while incorporating minor patches and bugfixes without waiting for a whole release.

acxz commented 4 years ago

Thanks for searching them up.

Agreed, submodules would be a cleaner solution than a hard copy of the 3rd party codebase.

ruffsl commented 4 years ago

From the main CmakeLists.txt, it also looks like gtsam attempts to find these libraries as well?

jlblancoc commented 4 years ago

Note: For Eigen3, there is already the GTSAM_USE_SYSTEM_EIGEN flag that should be set to ON in Debian pacakge builds so it's enforced NOT to use the embedded copy of that library.

Boost is also obviously taken from the system.

libboost-dev Could be narrowed down to only component packages actually used

I don't think it's worth the work at the cmake level: debuild tools will end up filling up the exact binary (.so) dependencies. And the header dependencies (what boost libraries are included into public headers) should be manually added to the debian/control file. Here, I agree it is worth not depending on all boost-dev but to be as narrow as possible.

dellaert commented 4 years ago

Quick note: neither me nor my students have cycles to go in depth on this, so if you all want changes and/or better documentation on this, we would love to see that but it’ll have to come from the community, I.e., you all :-) Do review each other’s PRs but tag me if there are controversial issues that you feel need my input. That includes everything that might affect existing users out there - anything that breaks existing installations should be a non-starter, IMHO.

varunagrawal commented 4 years ago

Here are all the git* links for future ease:

jlblancoc commented 4 years ago

(adding @berndpfrommer to this conversation)

berndpfrommer commented 4 years ago

I'm currently working on improved packaging for Ubuntu/Debian, and submitted a related PR #328

In order to get proper multi-architecture packaging working, we also need to take care of libmetis (see #309 and #220), which, when built with multi-architecture enabled, is currently colliding with the proper libmetis from the regular Ubuntu distros.

I agree the proper fix is to use the system libraries instead of the sources that are in 3rdparty. For now, the easiest workaround though is to call the privately built library libmetis-gtsam.so, and install it in the proper location, where it will no longer collide with libmetis.so. I have PR for this ready to go, and will submit it once #328 is approved/merged.

In a next step we have to find out what the motivation is for each of the packages in "gtsam/3rdparty", and why they are being used instead of the system libraries. I could imagine there being valid reasons, like avoiding having to debug with varying upstream versions of e.g. libmetis. Would be great to get rid of anything possible there so long as the GTSAM developers are fine with it.

jlblancoc commented 4 years ago

I created a new branch: feature/use-system-libraries, please let's open all PRs to address #292 against that branch. Per @dellaert 's advice, we should not change the default behavior until the next major release. But nothing prevents changes to make cmake to use system libraries depending on an option(). If that rule is observed, we could eventually merge the new branch into develop w/o problems, and some day, change the cmake defaults from "use embedded" to "use system".

For debian/rules, we can easily override those defaults for gtsam4 without problems.

That said... I would vote for treating Eigen differently and strongly prefer to use the system version by default, as already proposed in 07bace5e057d305674b0b17c47a2bc56bb7926a9 , since mixing Eigen versions may lead to obscure memory errors. Do you agree, @dellaert ? Note that this changes behavior, since the default until now has been to use the embedded copy of eigen3.... which probably was a good idea if much older versions were available in the past as system library.

acxz commented 4 years ago

Update on using system METIS:

One of our files: https://github.com/borglab/gtsam/blob/develop/gtsam_unstable/partition/FindSeparator-inl.h uses metislib.h. This header file is not exposed by upstream METIS, which is why we build METIS ourselves.

In order for us to use the system METIS, METIS needs to expose this functionality. I opened an issue & PR about this a while back (https://github.com/KarypisLab/METIS/issues/8 & https://github.com/KarypisLab/METIS/pull/9) and METIS's author has decided not to expose this functionality. https://github.com/KarypisLab/METIS/issues/8#issuecomment-656229647 & https://github.com/KarypisLab/METIS/pull/9#issuecomment-656230221

Therefore for us to use a system install of METIS, we need to either: 1) Change FindSeparator-inl.h such that metislib.h is not needed and only uses metis.h. At this point I am not familiar with the functionality of the file as to determine if this is even feasible. (However, I'm pretty sure we do need the metislib.h functionality.) 2) Spend more time convincing METIS's author 3) Go around METIS's author's wishes by adding the necessary patch at the Linux distribution level. I have enough clout with the Arch Linux team where this patch can be included in the Arch Linux METIS package, however for Debian/Ubuntu this would most likely involve a considerable amount of communication effort with those teams. (Not a scalable solution at all)

As of now I am closing #309 (which was based on having https://github.com/KarypisLab/METIS/pull/9#issuecomment-656230221 merged in upstream METIS) and slating down that GTSAM will not be able to use upstream METIS.

jlblancoc commented 4 years ago

1) would be the best path. In the worst case, we could forward declare the required types and/or C functions, and if the functions are exposed in metis.so, we could link against them without problems. That's a brittle solution though, the best indeed is determining why gtsam is using something that METIS' author thinks it shouldn't be using :-)

jlblancoc commented 3 years ago

@varunagrawal :

Here are all the git* links for future ease:

Please, add libcpputest-dev... and probably it's a good idea to edit the OP with the updated list with checkboxes to keep track of the progress on this important WIP task ;-)

varunagrawal commented 3 years ago

Done

jlblancoc commented 3 years ago

Thanks! Marking libeigen3 as done.

varunagrawal commented 3 years ago

@jlblancoc @acxz I went through METIS' documentation right now and it seems like @nikai and @amelim were trying to write a custom separator function for partitioning the graph based on edges. I've asked on the linked issue what the correct way to approach that would be that doesn't involve exposing the internals. Let's hope to hear a response soon.

jlblancoc commented 3 years ago

Great, thanks!

varunagrawal commented 3 years ago

I haven't heard back back on the Metis issue and considering that FindSeparator-inl.h is in unstable, I would recommend just killing it/commenting it out since we don't have a current need for it.

@dellaert your thoughts please?

berndpfrommer commented 3 years ago

Looked some more into this issue: I was able to build gtsam and gtsam_unstable using @jlblancoc's PR: https://github.com/borglab/gtsam/pull/309.

Some more digging reveals: the only two files pulling in (directly or indirectly) the offending header "libmetis.h" are:

/gtsam_unstable/partition/tests/testNestedDissection.cpp /gtsam_unstable/partition/tests/testFindSeparator.cpp

So both are tests. Wouldn't it then be just sufficient to disable those two tests?

ProfFan commented 3 years ago

@berndpfrommer Could you try removing these two tests and see if GTSAM can be built with system METIS? I remember @acxz has an old PR that does that. (Correct me if not)

acxz commented 3 years ago

I think you can modify #570 with my suggestions (using <> instead of "") and it should work. #309 worked but the cmake wasn't as clean.

Timple commented 2 years ago

That's the problem with parallel branches, they go out-of-date: feature/use-system-libraries

I was wondering why this package wasn't released yet to the ROS buildfarm. Would be nice, because rtabmap also has an optional dependency on it.

But just like ruffsl mentioned, a sub-optimal path is chosen just to ease the release process. Which is a pitty of such a nice project as gtsam.

wep21 commented 1 year ago

@jlblancoc Hi, is there any timeline to release this package as a ros2 apt package?

jlblancoc commented 1 year ago

@wep21 Just opened the first PR to release gtsam to ros2/rolling here: https://github.com/ros/rosdistro/pull/36277 Once it's merged, and their build farm processes it (feel free of pinging me here when you see the first build log from their Jenkins at https://build.ros2.org ) and everything looks fine, we could move on and make the release for the rest of active ros2 distros. Then, until the next "sync" the package won't be available for end users thru "apt", and that usually takes 1-2 months... It ain't easy nor fast :-)