Skip to content

Avoid compilation on jazzy or higher #301

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

Merged

Conversation

Rayman
Copy link
Contributor

@Rayman Rayman commented Jul 26, 2024

On jazzy the released openvdb version is already 10.0.1, so we don't have to compile the vendor package.

Compilation on the build farm was already quite flakey because the package is quite heavy, so I'd be best to avoid this. I don't know if openvdb is also released for ARM or any other ubuntu platforms, but we'll have to see on the buildfarm.

On jazzy the released openvdb version is already 10.0.1, so we don't
have to compile the vendor package.

Compilation on the build farm was already quite flakey because the
package is quite heavy, so I'd be best to avoid this. I don't know if
openvdb is also released for ARM or any other ubuntu platforms, but
we'll have to see on the buildfarm.
@SteveMacenski
Copy link
Owner

Do the released binaries on Jazzy / 24.04 work? They also existed on 20.04/22.04, but whoever actually did the distro releases added build flags that broke STVLs ability to use it.

@Rayman
Copy link
Contributor Author

Rayman commented Jul 28, 2024

I only tested that the software compiles. Maybe I'll have time to do some more testing next week

@SteveMacenski
Copy link
Owner

That’s worth checking. It compiled in 20/22 as well, but didn’t work at runtime

@Rayman
Copy link
Contributor Author

Rayman commented Aug 16, 2024

It seems like its working:

image

@SteveMacenski
Copy link
Owner

Oh that IS a good sign! What does the compute look like - in the range that you expect from Humble and prior? If so, this is absolutely the way to go for 24.04! We could (should?) remove the vendor package then as well so the build farm turns over and we don't waste builds that won't pass. Its still in the humble branch so if we ever needed to reintroduce it, its easy enough

@Rayman
Copy link
Contributor Author

Rayman commented Aug 19, 2024

I don't have any numbers for CPU at the moment

We could remove the vendor package, but right now its functioning as a backwards compatibility wrapper. So you can leave the package in the buildfarm (it won't compile anything).

@SteveMacenski
Copy link
Owner

SteveMacenski commented Aug 19, 2024

I don't see the point in using build resources for jazzy++ if the binaries work. As you've seen in the Discourse thread, this is causing problems so its worth removing it if we don't need it so we can get this released into jazzy and K-turtle on 24.04 without friction.

I won't remove it from the build farm for Humble / 22.04, but there is nothing working currently in 24.04 so its not removing what has never compiled in order to release 😉 Right now, every time I push it'll start trying to rebuild again. I can disable it in the release repository, but then the source makes it look like this is needed for ros2 / jazzy whereas it is not needed + is difficult to compile on Jetson devices. So, removing it I think is more clear to more passive on-lookers

I don't have any numbers for CPU at the moment

OK. Just ballpark it, I'm not looking for the difference in +/-3%, just general "yup, looks about the same". If there were a problem, you'd definitely see it in either the voxels not appearing or the compute resources skyrocketing.

@Timple
Copy link
Contributor

Timple commented Aug 19, 2024

I think @Rayman means keeping the vendor package but letting that find the binaries. So basically merge this PR. That won't consume any compilation cycles. Just a few file moves for the buildfarm.

@SteveMacenski
Copy link
Owner

Oh got it - I understand. Thanks for the clarification.

A sanity check on CPU I think is the last thing before we can merge + release, correct?

@Rayman
Copy link
Contributor Author

Rayman commented Aug 20, 2024

Left: main branch, right: my PR. The flamechart seems about the same. htop also is about the same

image

@SteveMacenski
Copy link
Owner

The middle spike looks somewhat substantive but its had for me to tell from that graphic without units. What's the total CPU number look like in comparison? (i.e. is that the difference in 1%, 10%, 100% of a core)

@Rayman
Copy link
Contributor Author

Rayman commented Aug 21, 2024

Total CPU seems about the same. The reason for the "spike" is that I could have slightly different compile settings, so there can be more/less inlining compared to the apt version.

htop on main:
main

htop on PR:
patch

@SteveMacenski
Copy link
Owner

0.3 & 0.1% is hardly a spike - that's well within the regime of noise

I'm happy with that. Anything else you want to do here? I can merge and release on Friday when I have my scheduled Nav2/STVL sync day

@Rayman
Copy link
Contributor Author

Rayman commented Aug 22, 2024

I think we've done enough research. Let's get it merged!

@SteveMacenski SteveMacenski merged commit 61d9d6a into SteveMacenski:ros2 Aug 22, 2024
@Rayman Rayman deleted the feature/avoid-compilation-on-jazzy branch August 23, 2024 11:15
SteveMacenski pushed a commit that referenced this pull request Aug 23, 2024
On jazzy the released openvdb version is already 10.0.1, so we don't
have to compile the vendor package.

Compilation on the build farm was already quite flakey because the
package is quite heavy, so I'd be best to avoid this. I don't know if
openvdb is also released for ARM or any other ubuntu platforms, but
we'll have to see on the buildfarm.
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 this pull request may close these issues.

3 participants