search

ros-perception / laser_geometry

Provides the LaserProjection class for turning laser scan data into point clouds.
BSD 3-Clause "New" or "Revised" License
156 stars 113 forks source link

Fix building on running on Windows Debug. #82

Closed clalancette closed 3 years ago

clalancette commented 3 years ago

In particular, we need to set the python executable properly when running on Windows Debug. While we are in here, we also fix up some dependencies in the package.xml and CMakeLists.txt.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should fix up the Windows Debug failures we are seeing after #80 was merged: https://ci.ros2.org/view/nightly/job/nightly_win_deb/2099/testReport/junit/(root)/laser_geometry/___/

@jonbinney FYI.

clalancette commented 3 years ago

CI:

clalancette commented 3 years ago

The Windows Debug failure is now different. Instead of failing to find the python interpreter, it timed out instead. I've kicked the timeout to 240 seconds, here's another CI run (just Windows Debug for now): Build Status

clalancette commented 3 years ago

OK, I spent some time with Windows Debug. Basically, we were running too many tests in laser_geometry. I've reduced the set of tests heavily now, so let's try CI on Windows Debug again:

Build Status

clalancette commented 3 years ago

Full CI again:

nuclearsandwich commented 3 years ago

I'm not thrilled by the idea of removing tests to make them pass. How long do the tests take with an absurd timeout? It seems odd to me that five extra asserts could be responsible for so much time, but I am not at all familiar with the project (and @blast545 points out that the time may come from the reduced scan range above). In most cases when tests are timing it is due to a hang or soft lock and not due to an exceptionally long test case so I think we should take a close look to make sure we aren't unintentionally papering over such a discovery.

Is it possible with pytest to label tests as performance or time intensive and only have them run if those tests are enabled? I understand the reasoning of not wanting to run these tests in the comprehensive ROS 2 CI for time but it would be nice if we could preserve them for package CI runs.

clalancette commented 3 years ago

I'm not thrilled by the idea of removing tests to make them pass. How long do the tests take with an absurd timeout?

They were taking about 500 seconds when running on my Windows VM. That is way too long for the buildfarm, since I don't want to add 10 minutes to the already absurdly long build times.

It seems odd to me that five extra asserts

It's not the asserts that are causing the speedup; I just removed those since they were redundant. What is really causing the speedup is the removal of the options from the lists in the test, like in ranges, intensities, etc. The test code takes the Cartesian product of all of those options, so each option there tends to add hundreds of tests. The removal of just a few of those options is responsible for a huge drop in the number of tests, and hence the test time.

And the thing is, we really don't need to run all of these tests (at least, not in this package). The Python portion of this package mostly offloads the computation to the sensor_msgs_py package, so we really only need to test just a few different options to get full "coverage" of the code here. Anything more than that is testing functionality in sensor_msgs_py (where we arguably should have more tests, but that is outside the scope of this change).

nuclearsandwich commented 3 years ago

Got it. Thanks for addressing concerns based on naive assumptions. I'm happy if other reviewers are.

clalancette commented 3 years ago

Based on reviews, I'm going to go ahead and merge this one in. This should get us green for Windows Debug. If there are any objections to this PR, please bring them up and I'm happy to do a follow-up to address them.