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

Upgrade stdgpu to latest version #7083

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Conversation

stotko
Copy link
Contributor

@stotko stotko commented Nov 30, 2024

Type

Motivation and Context

See detailed description below.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

Compilation with CUDA 12.4 and newer will fail due to breaking changes in thrust 2.3.0 (bundled with CUDA 12.4). In particular, there are 2 independent changes in thrust that broke compatibility with stdgpu:

  1. The previous hand-crafted and limited implementation of thrust::pair has been replaced by the more compliant cuda::std::pair implementation (see https://github.com/NVIDIA/cccl/releases/tag/v2.3.0). This has been fixed upstream in utility: Add custom pair implementation stotko/stdgpu#425 via a custom pair implementation.

  2. The implementation of thrust's namespace has been changed to include an ABI identifier which fixes linkage issues between different versions of thrust (see https://github.com/NVIDIA/cccl/releases/tag/v2.3.0 again). Since stdgpu needs to specialize some of thrust's internal classes to enable interoperability, this also broke compatibility. The issue has since also been fixed upstream in Fix thrust namespace for CUDA 12.6 stotko/stdgpu#428. However, the fix requires CUDA 11.5 or newer and has been advertised in General: Bump required thrust version to 1.13.1 stotko/stdgpu#429.

Thus, upgrade stdgpu to the latest version to enable support for CUDA 12.4 and newer. This also includes moving from thrust::pair to stdgpu::pair in the respective hash map backend and should be more future-proof. However, this also means that Open3D would need to bump its CUDA requirements in alignment with stdgpu to at least CUDA 11.5. I would argue that this is acceptable since:

  • Ubuntu 20.04 reaches EOL in < 6 months. The oldest CUDA version supporting Ubuntu 22.04 is CUDA 11.7.
  • The most recent MSVC compiler bumped the minimum required CUDA even from 11.6 to 12.4 (see Toolset update: VS 2022 17.10 Preview 2, WinSDK 22621, CUDA 12.4.0 microsoft/STL#4475).
  • New GPU generations need the most recent CUDA toolkit at the time of their release anyways, e.g. RTX 30XX --> 11.1, RTX 40XX --> 11.8, and the upcoming RTX 50XX --> 12.7/12.8 ?!

I only focused on the fix itself and have not yet updated the documentation or the CUDA version checks in CMake. This needs to be added if the proposed fix is deemed acceptable.

Further discussion and respective user bug reports (mostly on Windows) can be found here:

Copy link

update-docs bot commented Nov 30, 2024

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@stotko
Copy link
Contributor Author

stotko commented Nov 30, 2024

Failing CI runs look unrelated to this PR. Furthermore, the more relevant CUDA runs were skipped since this PR is from my fork. But I got positive replies from Windows users and tested the fix myself on Ubuntu 22.04, so I hope that's fine.

@patrikhuber
Copy link
Contributor

patrikhuber commented Dec 2, 2024

Big +1 from me for this PR - this is critical. Without it, Open3D doesn't compile on a recent CUDA version on MSVC. @ssheorey would you be able to review this or assign a reviewer to it, so that it can get merged?

@stotko I think you may also want to put #include <stdgpu/utility.h> at the top of the header with the changes?

@stotko
Copy link
Contributor Author

stotko commented Dec 3, 2024

@stotko I think you may also want to put #include <stdgpu/utility.h> at the top of the header with the changes?

Good catch, thanks. Should be included now.

@ssheorey ssheorey self-requested a review December 4, 2024 19:13
@ssheorey
Copy link
Member

ssheorey commented Dec 4, 2024

Thanks @stotko ! Are you also planning to update the docs / CUDA version checks?

@stotko
Copy link
Contributor Author

stotko commented Dec 5, 2024

Thanks @stotko ! Are you also planning to update the docs / CUDA version checks?

I can update them if you would like to have that in this PR as well. I first wanted some feedback on the fix itself before going too deep. Because, technically, this breaks compatibility with very old CUDA versions < 11.5.

@ssheorey
Copy link
Member

ssheorey commented Dec 5, 2024

Upgrading to CUDA 11.5+ sounds good. Please go ahead with the docs / cmake updates and we will gladly merge it.

@stotko
Copy link
Contributor Author

stotko commented Dec 6, 2024

Done. While updating the main CUDA version check within CMake, I have also checked for further occurrences and simplified some code that now does not need a version check anymore. Furthermore, I have set the "recommended" CUDA version to 12+ since recent versions of PyTorch and TensorFlow require it (or at least recommend it as well).

@ssheorey ssheorey merged commit b7e5f16 into isl-org:main Dec 6, 2024
29 of 38 checks passed
@stotko stotko deleted the stdgpu_upgrade branch December 6, 2024 14:55
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.

stdgpu target fails the CMake configure step with CUDA 12.4 due to changes in Thrust version header
3 participants