search

RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.33k stars 1.26k forks source link

Prepare for Eigen 3.4 #14968

Closed jwnimmer-tri closed 3 years ago

jwnimmer-tri commented 3 years ago

Eigen has entered the release candidate process for a 3.4.0 release. It's likely that Drake will incur some kinds of compilation errors due to the changes.

It's not clear yet how long it will be until a final release, but we should probably test against the candidates, and either adapt Drake to meet the new requirements, or else file Eigen upstream bugs if appropriate.

Since homebrew will immediately adopt Eigen 3.4.0 once it's tagged, we should try to get ahead of that failure mode.

Note, however, that we can probably test the Eigen 3.4.0 candidates on Ubuntu, even though the motivation here is to keep macOS working.

jwnimmer-tri commented 3 years ago

See #14876 for a report of one probable incompatibility.

jwnimmer-tri commented 3 years ago

Looking at https://eigen.tuxfamily.org/index.php?title=Main_Page and clicking to unfold the rc1 announcement, we see

Eigen 3.4-rc1 has been released on April 19, 2021. Depending on the amount of reported issues, 3.4 will be released late April or early May.

Upgrading priority.

jamiesnape commented 3 years ago

Depending on the amount of reported issues, 3.4 will be released late April or early May

Oh dear.

jamiesnape commented 3 years ago

https://drake-jenkins.csail.mit.edu/view/Production/job/mac-big-sur-unprovisioned-clang-bazel-nightly-release/70/consoleFull was a failed build on macOS with the RC (link should be available through mid-July).

BetsyMcPhail commented 3 years ago

PR #15142 was created to try out the 3.4 branch (6/7/2021) on both Linux and Mac. A summary of results with relevant CDash links to full output are listed below:

Test Errors

Build Errors

  1. ERROR: .../multibody/tree/BUILD.bazel:99:17: Compiling multibody/tree/multibody_tree.cc failed 
    multibody/tree/multibody_tree.cc:2939:36: error: no viable overloaded '='
Su(actuator_index, user_index) = 1.0;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~
  2. ERROR: .../common/trajectories/BUILD.bazel:105:17: Compiling common/trajectories/bspline_trajectory.cc failed 
    In file included from common/trajectories/bspline_trajectory.cc:1:
In file included from bazel-out/k8-opt/bin/common/trajectories/_virtual_includes/bspline_trajectory/drake/common/trajectories/bspline_trajectory.h:6:
In file included from bazel-out/k8-opt/bin/common/_virtual_includes/drake_bool/drake/common/drake_bool.h:5:
In file included from external/eigen/Eigen/Core:19:
external/eigen/Eigen/src/Core/util/Macros.h:1325:34: error: value of type 'Eigen::CwiseBinaryOp<Eigen::internal::scalar_cmp_op<double, double, Eigen::internal::cmp_EQ>, const Eigen::ArrayWrapper<const Eigen::Matrix<double, -1, -1, 0, -1, -1> >, const Eigen::ArrayWrapper<const Eigen::Matrix<double, -1, -1, 0, -1, -1> > >' is not contextually convertible to 'bool'
bool all(T t, Ts ... ts){ return t && all(ts...); }
  3. ERROR: .../solvers/BUILD.bazel:962:17: Compiling solvers/csdp_solver.cc failed 
    solvers/csdp_solver.cc:102:24: error: assigning to 'Eigen::DenseCoeffsBase<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 1>::Scalar' (aka 'double') from incompatible type 'typename internal::enable_if<(!IsRowMajor) && (!(internal::get_compile_time_incr<typename IvcType<TypeSafeIndex<FreeVariableTag> >::type>::value == 1 || internal::is_valid_index_type<TypeSafeIndex<FreeVariableTag> >::value)), IndexedView<const Matrix<double, -1, 1, 0, -1, 1>, typename IvcType<TypeSafeIndex<FreeVariableTag> >::type, IvcIndex> >::type' (aka 'Eigen::IndexedView<const Eigen::Matrix<double, -1, 1, 0, -1, 1>, drake::TypeSafeIndex<drake::solvers::internal::FreeVariableTag>, Eigen::internal::SingleRange>')
  (*prog_sol)(i) = s_val(std::get<SdpaFreeFormat::FreeVariableIndex>(
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  4. ERROR: .../multibody/plant/BUILD.bazel:490:20: Compiling multibody/plant/test/multibody_plant_reflected_inertia_test.cc failed 
    multibody/plant/test/multibody_plant_reflected_inertia_test.cc:88:48: error: no viable conversion from 'typename internal::enable_if<(!IsRowMajor) && (!(internal::get_compile_time_incr<typename IvcType<TypeSafeIndex<JointActuatorElementTag> >::type>::value == 1 || internal::is_valid_index_type<TypeSafeIndex<JointActuatorElementTag> >::value)), IndexedView<const Matrix<double, -1, 1, 0, -1, 1>, typename IvcType<TypeSafeIndex<JointActuatorElementTag> >::type, IvcIndex> >::type' (aka 'Eigen::IndexedView<const Eigen::Matrix<double, -1, 1, 0, -1, 1>, drake::TypeSafeIndex<drake::multibody::JointActuatorElementTag>, Eigen::internal::SingleRange>') to 'double'
  joint_actuator.set_default_rotor_inertia(rotor_inertias(index));
                                           ^~~~~~~~~~~~~~~~~~~~~
bazel-out/k8-opt/bin/multibody/tree/_virtual_includes/multibody_tree_core/drake/multibody/tree/joint_actuator.h:183:41: note: passing argument to parameter 'rotor_inertia' here
void set_default_rotor_inertia(double rotor_inertia) {
                                    ^
multibody/plant/test/multibody_plant_reflected_inertia_test.cc:89:45: error: no viable conversion from 'typename internal::enable_if<(!IsRowMajor) && (!(internal::get_compile_time_incr<typename IvcType<TypeSafeIndex<JointActuatorElementTag> >::type>::value == 1 || internal::is_valid_index_type<TypeSafeIndex<JointActuatorElementTag> >::value)), IndexedView<const Matrix<double, -1, 1, 0, -1, 1>, typename IvcType<TypeSafeIndex<JointActuatorElementTag> >::type, IvcIndex> >::type' (aka 'Eigen::IndexedView<const Eigen::Matrix<double, -1, 1, 0, -1, 1>, drake::TypeSafeIndex<drake::multibody::JointActuatorElementTag>, Eigen::internal::SingleRange>') to 'double'
  joint_actuator.set_default_gear_ratio(gear_ratios(index));
                                        ^~~~~~~~~~~~~~~~~~
bazel-out/k8-opt/bin/multibody/tree/_virtual_includes/multibody_tree_core/drake/multibody/tree/joint_actuator.h:189:38: note: passing argument to parameter 'gear_ratio' here
void set_default_gear_ratio(double gear_ratio) {
  5. ERROR: .../multibody/inverse_kinematics/BUILD.bazel:257:20: Compiling multibody/inverse_kinematics/test/global_inverse_kinematics_reachable_test.cc failed 
    multibody/inverse_kinematics/test/global_inverse_kinematics_reachable_test.cc:80:17: error: no viable conversion from 'typename internal::enable_if<(!IsRowMajor) && (!(internal::get_compile_time_incr<typename IvcType<TypeSafeIndex<JointElementTag> >::type>::value == 1 || internal::is_valid_index_type<TypeSafeIndex<JointElementTag> >::value)), IndexedView<Matrix<double, 7, 1, 0, 7, 1>, typename IvcType<TypeSafeIndex<JointElementTag> >::type, IvcIndex> >::type' (aka 'Eigen::IndexedView<Eigen::Matrix<double, 7, 1, 0, 7, 1>, drake::TypeSafeIndex<drake::multibody::JointElementTag>, Eigen::internal::SingleRange>') to 'double'
EXPECT_NEAR(q_global_ik(plant_->GetJointByName("iiwa_joint_2").index()),
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  6. ERROR: .../math/BUILD.bazel:496:20: Compiling math/test/rotation_conversion_test.cc failed 
    math/test/rotation_conversion_test.cc:174:34: error: 'LinSpaced' is deprecated [-Werror,-Wdeprecated-declarations]
auto roll = Eigen::VectorXd::LinSpaced(Eigen::Sequential, kSweepSize,

    7. ERROR: .../multibody/fixed_fem/dev/BUILD.bazel:296:17: Compiling multibody/fixed_fem/dev/fem_model_base.cc failed

    In file included from multibody/fixed_fem/dev/fem_model_base.cc:1:
In file included from bazel-out/k8-opt/bin/multibody/fixed_fem/dev/_virtual_includes/fem_model_base/drake/multibody/fixed_fem/dev/fem_model_base.h:11:
bazel-out/k8-opt/bin/multibody/fixed_fem/dev/_virtual_includes/dirichlet_boundary_condition/drake/multibody/fixed_fem/dev/dirichlet_boundary_condition.h:111:30: error: no viable overloaded '='
  (*residual)(dof_index) = 0;
  ~~~~~~~~~~~~~~~~~~~~~~ ^ ~
BetsyMcPhail commented 3 years ago

Fixes and workarounds are in PR #15142. There is currently one build error and 20 failing tests. Results are the same across all platforms and configurations.

To test locally, checkout branch from PR and revert referenced commit. To sanity check against Eigen 3.3, update eigen/repository.bzl to branch = "3.3.9" instead of branch = "3.4"

The following tests are failing without much output in the normal test results. Leak sanitizer results may provide some clues?

BetsyMcPhail commented 3 years ago

@jwnimmer-tri it would be helpful to have some Eigen experts dig into these failures.

jwnimmer-tri commented 3 years ago

@BetsyMcPhail

These three completed checkbox commits seem like good fixes. Could you PR each one to Drake individually, so that we can get them merged and out of the way to narrow down the branch?

You can assign these two to me for review.

This one looks correct at first glance, but please assign to @soonho-tri to review just to be sure.

jwnimmer-tri commented 3 years ago

EIGEN_INHERIT_ASSIGNMENT_EQUAL_OPERATOR error (temp. workaround to get tests running 8a3532d, need an actual solution)

I spent a few minutes investigating, and I don't think there is a good systemic fix. I think a more polished version of your current work-around is the best way to go, as follows.

For lines like this, which fail because an Eigen::Matrix lookup uses a TypeSafeIndex (in this case, the actuator.index()) ...

B(actuator.joint().velocity_start(), actuator.index()) = 1;

... we just need to guide the implicit conversion to int, like so ...

B(actuator.joint().velocity_start(), int(actuator.index())) = 1;

Note that I use the int constructor int(...) instead of static_cast<int>(...), and that I keep it inline instead of factoring it into a different named variable just to change the type.

I suspect that more brief approach will lead to a slightly more readable patch, that would be acceptable to merge to master now. Could you try opening a PR with that?

jwnimmer-tri commented 3 years ago

Build error: 'static_assert failed ...' ... cassie_benchmark test failures

I suspect that our forked copy of common/autodiffxd.h might be incompatible with Eigen 3.4. Maybe I can nominate @rpoyner-tri to root cause and fix these two failures.

Edit: Filed as https://github.com/RobotLocomotion/drake/issues/15401 now.

jwnimmer-tri commented 3 years ago

Any tests that are failing related to symbolic::Expression are probably due to #15298. We could apply that work-around to the branch for now, to help clear the deck of those errors until upstream gets fixed.

BetsyMcPhail commented 3 years ago

Any tests that are failing related to symbolic::Expression are probably due to #15298. We could apply that work-around to the branch for now, to help clear the deck of those errors until upstream gets fixed.

Sounds good to me. I will apply the patch now.

BetsyMcPhail commented 3 years ago

EIGEN_INHERIT_ASSIGNMENT_EQUAL_OPERATOR error (temp. workaround to get tests running 8a3532d, need an actual solution)

I spent a few minutes investigating, and I don't think there is a good systemic fix. I think a more polished version of your current work-around is the best way to go, as follows.

For lines like this, which fail because an Eigen::Matrix lookup uses a TypeSafeIndex (in this case, the actuator.index()) ...

B(actuator.joint().velocity_start(), actuator.index()) = 1;

... we just need to guide the implicit conversion to int, like so ...

B(actuator.joint().velocity_start(), int(actuator.index())) = 1;

Note that I use the int constructor int(...) instead of static_cast<int>(...), and that I keep it inline instead of factoring it into a different named variable just to change the type.

I suspect that more brief approach will lead to a slightly more readable patch, that would be acceptable to merge to master now. Could you try opening a PR with that?

Using just the int constructor makes cpplint unhappy, For example, in multibody_plant.cc, 

--- a/multibody/plant/multibody_plant.cc
+++ b/multibody/plant/multibody_plant.cc
@@ -768,7 +768,7 @@ MatrixX<T> MultibodyPlant<T>::MakeActuationMatrix() const {
     // This method assumes actuators on single dof joints. Assert this
     // condition.
     DRAKE_DEMAND(actuator.joint().num_velocities() == 1);
-    B(actuator.joint().velocity_start(), actuator.index()) = 1;
+    B(actuator.joint().velocity_start(), int(actuator.index())) = 1;
   }
   return B;
 }

Results in style error:

==================== Test output for //multibody/plant:py/multibody_plant_core_cpplint:
multibody/plant/multibody_plant.cc:771:  Using deprecated casting style.  Use static_cast<int>(...) instead  [readability/casting] [4]
jwnimmer-tri commented 3 years ago

Oops. What about int{actuator.index()} with curlies instead?

jwnimmer-tri commented 3 years ago

At this point, I think this meta-issue isn't useful anymore, so I'll close it.

We'll leave #15142 open as our canary branch. Once it's passing all CI (even post-merge) with zero diffs vs master other than using Eigen 3.4 in the repository rule, then our job is done.

As we identify acute problems in the canary branch, we'll open specific issues for them.