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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Kokkos v4.5.01 #692

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Upgrade to Kokkos v4.5.01 #692

merged 3 commits into from
Jan 6, 2025

Conversation

tpadioleau
Copy link
Member

@tpadioleau tpadioleau commented Nov 29, 2024 •
edited
Loading

  • update Kokkos to v4.5.01
    • try release candidate
    • update find_package range version
  • update Kokkos Kernels to v4.5.01
    • try release candidate
    • update Pbtrs (nothing)
    • update Pttrs (nothing)
    • update find_package range version
    • copy Gbtrs and Getrs in kernels/splines/kokkos-kernels-ext
    • put the copyrights of DDC, disable clang-format and linting
    • replace Algo::G[be]trs::Unblocked by Algo::Level3::Unblocked

@tpadioleau tpadioleau self-assigned this Nov 29, 2024
@tpadioleau
Copy link
Member Author

Reported in kokkos/mdspan#367

@tpadioleau tpadioleau marked this pull request as draft December 2, 2024 18:01
@tpadioleau
Copy link
Member Author

back to draft until Kokkos patch 4.5.01

@tpadioleau tpadioleau force-pushed the upgrade-to-kokkos-4.5.00 branch 2 times, most recently from 6a56876 to 1d4931d Compare December 5, 2024 09:57
@tpadioleau tpadioleau force-pushed the upgrade-to-kokkos-4.5.00 branch 2 times, most recently from 129f148 to 755d7f9 Compare December 13, 2024 18:34
@tpadioleau tpadioleau changed the title Upgrade to Kokkos v4.5.00 Upgrade to Kokkos v4.5.01 Dec 13, 2024
@tpadioleau
Copy link
Member Author

tpadioleau commented Dec 17, 2024 •
edited
Loading

@yasahi-hpc What do you think of transferring the getrs function in DDC so that we don't have to rely on a fork of KK ? Just the time it gets upstreamed.

@yasahi-hpc
Copy link
Member

@yasahi-hpc What do you think of transferring the getrs function in DDC so that we don't have to rely on a fork of KK ? Just the time it gets upstreamed.

I think it is better.

@tpadioleau tpadioleau force-pushed the upgrade-to-kokkos-4.5.00 branch 14 times, most recently from 79e2d84 to 0769aea Compare December 24, 2024 09:02
@tpadioleau tpadioleau marked this pull request as ready for review December 24, 2024 09:02
@tpadioleau tpadioleau force-pushed the upgrade-to-kokkos-4.5.00 branch 2 times, most recently from 8430c6b to 72ab0d5 Compare December 28, 2024 16:29
yasahi-hpc
yasahi-hpc previously approved these changes Dec 29, 2024
View reviewed changes
Copy link
Member

@yasahi-hpc yasahi-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me.

  1. It seems that you strictly need Kokkos 4.5.01 or later. In CMake, should we require 4.5.01 as minimum?
  2. It may be done in another PR, but could you please describe the minimum requirement of DDC in the documentation? Like CMake 3.22+, Kokkos 4.5.01+, etc.
  3. Could you leave comments somewhere that kokkos-kernels-ext will be removed as soon as they are upstreamed to KK?

CMakeLists.txt
@@ -48,15 +48,15 @@ set(DDC_Kokkos_DEPENDENCY_POLICY
set_property(CACHE DDC_Kokkos_DEPENDENCY_POLICY PROPERTY STRINGS "${DDC_DEPENDENCY_POLICIES}")
if("${DDC_Kokkos_DEPENDENCY_POLICY}" STREQUAL "AUTO")
if(NOT TARGET Kokkos::kokkos)
find_package(Kokkos 4.4...4.5 QUIET)
find_package(Kokkos 4.4...<5 QUIET)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
find_package(Kokkos 4.4...<5 QUIET)
find_package(Kokkos 4.5...<5 QUIET)

As found in the title of this PR, we need 4.5.01 or later right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am hesitating:

  • 4.4.* should be working as we are not using any specific feature from 4.5.*
  • 4.5.0 does not work due to the typo issue in mdspan
  • 4.5.1 works again

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure
Then, relying on 4.5.1?
I would like to disallow 4.5.0 since it does not work.

find_package(Kokkos 4.5.1...<5 QUIET)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a shame we cannot claim we support 4.4.

Copy link
Member

@yasahi-hpc yasahi-hpc Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I am fine either one of these

  1. support 4.4 in CMake + documentation of supported versions (4.5 is not available)
  2. support 4.5.1 in CMake (do not allow 4.5)

Documentation may be updated in another PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a "known issues" section in the README.md

CMakeLists.txt Show resolved Hide resolved
cmake/DDCConfig.cmake.in Show resolved Hide resolved
@tpadioleau
Copy link
Member Author

Thanks,

  1. It may be done in another PR, but could you please describe the minimum requirement of DDC in the documentation? Like CMake 3.22+, Kokkos 4.5.01+, etc.

Yes I will do that

  1. Could you leave comments somewhere that kokkos-kernels-ext will be removed as soon as they are upstreamed to KK?

I can open an issue ?

As we are claiming a compatibility 4.5...<5 for Kokkos Kernels, if we upstream new solvers in 4.6, we will likely face a clash of symbols for Gbtres/Getrs because they are in the namespace KokkosKernels. Maybe I should put them in the namespace ddc::KokkosKernels ?

@yasahi-hpc
Copy link
Member

  1. Could you leave comments somewhere that kokkos-kernels-ext will be removed as soon as they are upstreamed to KK?

I can open an issue ?

As we are claiming a compatibility 4.5...<5 for Kokkos Kernels, if we upstream new solvers in 4.6, we will likely face a clash of symbols for Gbtres/Getrs because they are in the namespace KokkosKernels. Maybe I should put them in the namespace ddc::KokkosKernels ?

I think they will be placed under KokkosBatched namespace as well as other solvers like pbtrs and pttrs. You should just change the include file and the minimum requirement to 4.6.

@tpadioleau
Copy link
Member Author

I think they will be placed under KokkosBatched namespace as well as other solvers like pbtrs and pttrs. You should just change the include file and the minimum requirement to 4.6.

If I put the solvers in ddc::KokkosKernels we could support Kokkos Kernels from 4.5.

@yasahi-hpc
Copy link
Member

I think they will be placed under KokkosBatched namespace as well as other solvers like pbtrs and pttrs. You should just change the include file and the minimum requirement to 4.6.

If I put the solvers in ddc::KokkosKernels we could support Kokkos Kernels from 4.5.

If you would like to support KK 4.5, I would prefer to switch the include file by macro like

#if KOKKOSKERNELS_VERSION >= 40599
#include <KokkosBatched_Gbtrs.hpp>
#else
#include "kokkos-kernels-ext/KokkosBatched_Gbtrs.hpp"
#endif

Anyway, this matters only after solvers are upstreamed to KK in 4.6.

@tpadioleau tpadioleau force-pushed the upgrade-to-kokkos-4.5.00 branch from 72ab0d5 to 13dec09 Compare January 5, 2025 08:58
@tpadioleau tpadioleau force-pushed the upgrade-to-kokkos-4.5.00 branch from 13dec09 to 037628d Compare January 5, 2025 12:29
@tpadioleau tpadioleau requested a review from yasahi-hpc January 6, 2025 10:52
@tpadioleau
Copy link
Member Author

tpadioleau commented Jan 6, 2025 •
edited
Loading

@yasahi-hpc do you agree that the current version should still compile fine even if getrs and gbtrs get merged in 4.6 ?

@yasahi-hpc
Copy link
Member

@yasahi-hpc do you agree that the current version should still compile fine even if getrs and gbtrs get merged in 4.6 ?

If you keep relying on the internal implementations, yes it should compile fine. If you would switch to the KK 4.6 implementations, it should still be fine, but I am not 100 % sure.

@tpadioleau tpadioleau merged commit 88c9392 into main Jan 6, 2025
56 checks passed
@tpadioleau tpadioleau deleted the upgrade-to-kokkos-4.5.00 branch January 6, 2025 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yasahi-hpc yasahi-hpc Awaiting requested review from yasahi-hpc

Assignees

@tpadioleau tpadioleau

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tpadioleau @yasahi-hpc