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

Eigen version issue #19

Open
oleg-alexandrov opened this issue Jul 14, 2021 · 10 comments
Open

Eigen version issue #19

oleg-alexandrov opened this issue Jul 14, 2021 · 10 comments

Comments

@oleg-alexandrov
Copy link

oleg-alexandrov commented Jul 14, 2021

The CERES solver is quite particular about Eigen. If a conda forge recipe for CERES is built with a certain version of Eigen, and my conda environment has a different version of Eigen, then when I build a package which depends on CERES it will fail.

Here is the source of the problem: https://github.com/ceres-solver/ceres-solver/blob/ec4f2995bbde911d6861fb5c9bb7353ad796e02b/cmake/CeresConfig.cmake.in#L200

The only solution I can think of is for conda packages for CERES to exist for multiple versions of Eigen, and each such conda package should make it very clear what version of Eigen it will work with. As of now however meta.yml has no version of Eigen, which makes the CERES conda package install well but then things fail later.

@Tobias-Fischer
Copy link
Contributor

Indeed we're running into issues when building cartographer at the moment: https://github.com/conda-forge/cartographer-feedstock/pull/20/checks?check_run_id=3629013766

Although we have a run_export, an incompatible version of eigen is pulled in. Any ideas why that is @conda-forge/ceres-solver @wolfv @traversaro?

@traversaro
Copy link

Although we have a run_export, an incompatible version of eigen is pulled in. Any ideas why that is @conda-forge/ceres-solver @wolfv @traversaro?

The pin_compatible directive works fine, i.e. if you open the info/run_exports.json file in the ceres package you find:

{"weak": ["ceres-solver >=2.0.0,<2.1.0a0", "eigen >=3.3.9,<3.3.10.0a0"]}

However, this will only ensure that any package that depends on ceres will have eigen >=3.3.9,<3.3.10.0a0 as run dependency, it does not enforces anything on the version to be installed in an environment in which only ceres is installed, such as the build environment of cartographer. Not sure if this can be considered a bug of how pin_compatible works, or we should simply also have similar constriants with eigen as run dependency of ceres.

However, more in general I am bit puzzled by this check of ceres. In all the conda-forge-related packaging efforts we have being doing for robotics, we always implicitly assumed to be all Eigen 3.* release to never introduce ABI incompatibilities with respect to earlier versions. Note that with "ABI incompatibilities" I mean the ABI of downstream compiled libraries that use Eigen in their public interface, as Eigen by itself does not have an ABI as it is an header-only library. This is probably going to be a big problem if an ABI-incompatible Eigen is going to be released (Eigen 4?), but for the time being never gave problems, at least as I am able to remember. If this is not true, and instead Eigen is ABI incompatible even at the patch level, this is really a ticking bomb. However, I tried to find the exact problem that lead to add this check in ceres, and I was able to only find these commits:

  • ceres-solver/ceres-solver@78cc2c4 This is a large CMake refactor, that however "silently" (i.e. no mention of it in the commit message) adds the EXACT option to find_package(Eigen ...), effectively requiring Eigen version used to build downstream projects to be exactly the one used by Ceres.
  • ceres-solver/ceres-solver@3527c0e This commit does not change the check, but clarify the error message. However, no specific detail on which Eigen version have ABI incompatibility (beside a reference to ODR violations) are given.

@Tobias-Fischer
Copy link
Contributor

See also conda-forge/eigen-feedstock#32

@Tobias-Fischer
Copy link
Contributor

Thanks for your analysis, @traversaro!

From reading what you say, I suggest there are two ways forward:

  1. Removing the check in ceres-solver that enforces the exact same version of Eigen
  2. Adding the constraint on the exact patch level of eigen to the run dependencies of ceres-solver

Considering that - as you said - in all other robotics related packages we assume that Eigen 3.* does not introduce ABI incompatibilities, my gut feeling is that we should go with 1).

Thoughts?

@traversaro
Copy link

  1. at least is a way to understand if the check is actually necessary or not. The nice thing is that they mention ODR-related problems, so any related problem should be at compilation level instead of runtime, so easier to detect. So unless someone has objections, it seems that 1) could be a good idea.

@oleg-alexandrov
Copy link
Author

Indeed, all Eigen versions are rather compatible and option 1 seems to be the best. Option 2 would be a little terrible, as for large projects it can result in conflicts. If option 2 is implemented there needs to be released one version of ceres for each version of Eigen.

@jschueller
Copy link
Contributor

or just bump ceres package build number when eigen package is upgraded, that's what I did recently

@traversaro
Copy link

or just bump ceres package build number when eigen package is upgraded, that's what I did recently

Indeed, this can be done easily and does not prevent to do everything else discussed in this issue later, let's do it while we wait for feedback on this issue: #22 .

@traversaro
Copy link

Indeed in conda-forge/dartsim-feedstock#16 we experienced a problem that seems to be related to Eigen ABI (even if it appears only on Windows):

  • When dartsim v6.11.0 is built with Eigen v3.4.0 and fcl v0.6.1 (that is build with Eigen v3.3.9), some tests fails. As soon as Eigen is downgraded to v3.3.9, the test pass instead.

@traversaro
Copy link

Possible fix: conda-forge/eigen-feedstock#41 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants