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

feat(autoware.repos): added ros2_spconv #5658

Closed
wants to merge 3 commits into from

Conversation

knzo25
Copy link
Contributor

@knzo25 knzo25 commented Jan 15, 2025

Description

Temporarily adds https://github.com/knzo25/ros2_spconv
to the autoware.repos file to test compilation times

This PR is required by:

How was this PR tested?

Notes for reviewers

None.

Effects on system behavior

None.

…n time

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Copy link

github-actions bot commented Jan 15, 2025

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@xmfcx xmfcx added the tag:run-health-check Run health-check label Jan 15, 2025
@xmfcx
Copy link
Contributor

xmfcx commented Jan 15, 2025

It failed without CUDA. Default behavior in Autoware packages is to skip building the CUDA packages yet report success for build 🤷 (like in autoware_lidar_centerpoint/CMakeLists.txt)

@knzo25
Copy link
Contributor Author

knzo25 commented Jan 15, 2025

@xmfcx
In this case, it seems to be because the second line of the cmakelist uses project(spconvlib LANGUAGES CXX CUDA).
By selecting it as a language, the rest of the cmake commands act different, which was needed to port the original cmake without many changes. I will try to find a way that does not make to much changes on the autogenerated code yet it is compliant with our way to do things 🙏

@knzo25
Copy link
Contributor Author

knzo25 commented Jan 27, 2025

@xmfcx
If I understood correctly, we are moving cuda packages outside universe, so this PR will probably become stalled.
The other alternative that was discussed was to make a deb package, which I created here:
https://github.com/knzo25/spconv_cpp

@knzo25
Copy link
Contributor Author

knzo25 commented Feb 4, 2025

@xmfcx @mitsudome-r
Although I could not be present in the last meetings due to experiments, etc
I was told that regarding spconv, we could add it as a normal ros repo (that takes long to compile) or prepare a deb package.
Since the CI/CD does not seem like it will work (please correct me if I am wrong), I also prepared a .deb package. If that is still an option, how can we distribute it? I just pasted above the repo that generates the .deb files

Currently, spconv would allow me to send PRs to BEVFusion (already sent and spconv is blocking it) and pointcloud semantic segmentation (PTv3)

@xmfcx
Copy link
Contributor

xmfcx commented Feb 14, 2025

Edit:
Interestingly https://s3.amazonaws.com/autonomoustuff-repo/ pacmod is also hosted on s3 as an apt repository. I wonder how hard it is to manage something like this would be considering we will have only a single package.

@esteve do you have any suggestions on this issue?

@esteve
Copy link
Contributor

esteve commented Feb 14, 2025

@xmfcx I think the effort of hosting one package or multiple ones is the same, it wouldn't make a difference in this case.

@knzo25 have you tried using https://cmake.org/cmake/help/latest/module/CheckLanguage.html#module:CheckLanguage and then https://cmake.org/cmake/help/latest/command/enable_language.html ? You could make it conditional, enable_language and project(PROJ LANG...) are equivalent.

@knzo25
Copy link
Contributor Author

knzo25 commented Feb 17, 2025

@xmfcx

I will close this one if/once the other gets merged.

I created a release in github, which allows ansible to download the package, while also addressing to some degree the versioning concerns (added the version similarly to cuda/tensorrt). If we decide to go with this, I will transfer the repo to the awf.
https://github.com/knzo25/spconv_cpp/releases

@esteve
The repo itself is mostly auto-generated code from upstream, and as such, I would like to keep the diffs as little as possible. Does the current way to do things cause you any issues? 🤔

@knzo25
Copy link
Contributor Author

knzo25 commented Feb 22, 2025

Closing in favor of
#5794

@knzo25 knzo25 closed this Feb 22, 2025
@esteve
Copy link
Contributor

esteve commented Feb 22, 2025

@knzo25 I see. I can always skip this repository in our buildfarm, but ideally it'd be good that the package build fails gracefully if it can't find CUDA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag:run-health-check Run health-check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants