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

Release binaries of px4_msgs due to long compilation time #41

Open
Ryanf55 opened this issue Aug 1, 2024 · 11 comments
Open

Release binaries of px4_msgs due to long compilation time #41

Ryanf55 opened this issue Aug 1, 2024 · 11 comments
Assignees

Comments

@Ryanf55
Copy link

Ryanf55 commented Aug 1, 2024

Request

Please consider releasing tagged binaries for px4_msgs.
The CI time for this package on the last job was over 5 hours.
Github's limit for their runners is 6 hours
Because binaries are not available, I must build this from source, and it takes a long time.

Problems encountered

Because binaries are not available, I must build this from source, and it takes a long time.
I'm limited to 2 hours per job on the CI solution I have.

Proposed Solutions

  • Release binaries to ROS 2 humble using these instructions
  • Add ccache to the Gihtub environment (only useful for those forking this in Github)
  • Achieve binaries listed on the [ROS index for PX4[(https://index.ros.org/p/px4_msgs/github-PX4-px4_msgs/#humble)

Complexities

Because PX4 does not promise a message-stable ABI within an LTS ROS distro, I am not sure whether the ROS build farm will allow you to support multiple versions, or how this will work with rosdep's lack of versioning support. Maybe this will require PX4 to host a PPA and users can run sudo apt install ros-humble-px4-msgs-f31e301b3, where that corresponds to this build hash; f31e301

Anyone using px4_msgs in a CI pipeline outside of Github could help provide expertise in how they deal with this.

@afxalz
Copy link

afxalz commented Aug 1, 2024

I am having a lot trouble building the package. The compile time along with the debug time is quite large. Releasing the binaries would be extremely useful to accelerate the development process. I think the community will greatly appreciate this help.

@dagar
Copy link
Member

dagar commented Aug 1, 2024

I'm not opposed to packaging if we do it corresponding to PX4 Autopilot stable releases (v1.14.x, etc). What would be the right way to package/release px4_msgs corresponding to different versions of PX4 simultaneously? Would they each need to be a distinct interface package with version naming or could we do it such that they all provide px4_msgs, but they can't be installed simultaneously.

We could also easily limit px4_msgs to the small subset people are even using externally. For example here's what we currently have by default for microXRCE-DDS. https://github.com/PX4/PX4-Autopilot/blob/75ce550db39cc5d61ba970ca5f4b6096a287827e/src/modules/uxrce_dds_client/dds_topics.yaml

The potential downside is that I want it to be super easy for developers to privately add whatever random message they might want, but I think we could start with a reasonable guess and still cut it down the absurd build time by like 80-90%.

@Jaeyoung-Lim @beniaminopozzan @TSC21

@dagar
Copy link
Member

dagar commented Aug 1, 2024

Because binaries are not available, I must build this from source, and it takes a long time.
I'm limited to 2 hours per job on the CI solution I have.

@Ryanf55 separately from my comment above, have you tried building with ninja instead of make?

$ colcon build --cmake-args " -GNinja"

@dagar dagar self-assigned this Aug 1, 2024
@Ryanf55
Copy link
Author

Ryanf55 commented Aug 1, 2024

I'm not opposed to packaging if we do it corresponding to PX4 Autopilot stable releases (v1.14.x, etc). What would be the right way to package/release px4_msgs corresponding to different versions of PX4 simultaneously? Would they each need to be a distinct interface package with version naming or could we do it such that they all provide px4_msgs, but they can't be installed simultaneously.

We could also easily limit px4_msgs to the small subset people are even using externally. For example here's what we currently have by default for microXRCE-DDS. https://github.com/PX4/PX4-Autopilot/blob/75ce550db39cc5d61ba970ca5f4b6096a287827e/src/modules/uxrce_dds_client/dds_topics.yaml

The potential downside is that I want it to be super easy for developers to privately add whatever random message they might want, but I think we could start with a reasonable guess and still cut it down the absurd build time by like 80-90%.

@Jaeyoung-Lim @beniaminopozzan @TSC21

I don't have an easy answer on how to break ABI during a distro. It's not part of the standard ROS way of doing it, so I recommend we talk to some of the infrastructure maintainers at ROS on how to accomplish that in a way that they would accept on their build farm.

The way that ROS handles when you have different message definitions is extremely poor (silently accepting junk data, or runtime crashes), so we have to make sure it's super obvious to users how to keep PX4 and your messages compatible.

I'll report back in Ninja can speed it up, I was assuming we already had that enabled everywhere internally.

@Jaeyoung-Lim
Copy link
Member

Jaeyoung-Lim commented Aug 1, 2024

@Ryanf55 Just be aware that px4_msgs are not supposed to be stable, and the expectation is that every px4 commit will break the compatibility of px4_msgs.

Meaning: every point release needs its own packaged distribution

@Ryanf55
Copy link
Author

Ryanf55 commented Aug 1, 2024

Absolutely, I get that PX4 wants to change messages. Totally fine.

Here's a proposed workflow:

I have a repos file:

repositories:
   px4_msgs:
      type: git
      url: [email protected]:AVACore/publicexternals/px4_msgs.git
      version: v1.14.x
   PX4-Autopilot:
      type: git
      url: [email protected]:AVACore/publicexternals/PX4-Autopilot.git
      version: v1.14.x

When I
rosdep install --from-paths src
Then, I expect the following to occur:
apt install ros-humble-px4-msgs-v1.14
Tomorrow, ros-humble-px4-msgs-v1.15 could be released that breaks ABI, but this workflow still installs 1.14.

Although with a PPA you could do apt install ros-humble-px4-msgs~1.14, rosdep doesn't support versioning to apt.

Then, in CI, when I run colcon build, I can do a colcon build --packages-skip px4_msgs_v1.14 because it's available through apt.

Headers could be installed as px4_msgs/msg/MyMsg.hpp to preserve existing header paths.

We would need to decide whether you do find_package(px4_msgs CONFIG 1.14) or find_package(px4_msgs_v1.14)

@Ryanf55
Copy link
Author

Ryanf55 commented Aug 2, 2024

As far as taking the route of limiting which messages are generated, what about adding CMake options for PX4_MIN_MSGS and PX4_ALL_MSGS.

PX4_ALL_MSGS is the list of all interfaces, while MIN is only the ones used by XRCE DDS.

In order for a user to choose which messages, they set a variable PX4_GENERATED_MSGS to either MIN or ALL, and they can also override/remove/append items to those lists if they want to remove or add messages.

Then default build could be all messages, but now we have a one-liner through --cmake-args to only generate the interfaces used by ROS and cut the build time down.

@dagar
Copy link
Member

dagar commented Aug 2, 2024

If you're suggesting adding that here (PX4/px4_msgs) that sounds like a harmless workaround for now.
I was talking about only extracting the minimum (or reasonable) subset from PX4-Autopilot in the first place.

@Ryanf55
Copy link
Author

Ryanf55 commented Aug 7, 2024

It looks like colcon already enables building with all cores and a parallel build, regardless of which generator you use.
https://robotics.stackexchange.com/questions/97896/colcon-build-number-of-threads

I checked why the CI is taking 5 hours here, and did not see any obvious problems in the Github actions used.

@beniaminopozzan
Copy link
Member

While it is true that binaries for this package would be great, the 6h build time on the CI were caused by ros-tooling/action-ros-ci running tests after compiling.
I did not investigate where exacly the issue is, but adding skip-test (https://github.com/ros-tooling/action-ros-ci?tab=readme-ov-file#skip-tests) reduced the CI time to 11 minutes.
PR for the fix is #34

@asherikov
Copy link

@beniaminopozzan it would be better to drop ADD_LINTER_TESTS in https://github.com/PX4/px4_msgs/blob/main/CMakeLists.txt#L32C2-L32C18 instead, it is a more general workaround, not specific to github. To my understanding those are more about testing the code generator rather than the messages.

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

No branches or pull requests

6 participants