-
Notifications
You must be signed in to change notification settings - Fork 100
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
Global CMake for tutorials #91
base: main
Are you sure you want to change the base?
Conversation
One difficulty is that the first 3-4 exercises are not perfect Kokkos code and can have issue when a Kokkos device backend is enabled. |
This way, the CMakeLists.txt is almost standard. Remove old common.cmake
advanced_reductions.cpp and parallel_scan.cpp cannot compile due to missing code (the exercise).
Use "" for include instead of <> to avoid adding `-I.` flag.
Still have to check on a Cuda computer.
This allows to not pollute the source directory.
I will add Kokkos-Kernels tutorials before merge, but I want to know other thoughts before continuing. A lot of files are touched, but their modifications are pretty similar. |
FetchContent_Declare( | ||
Kokkos | ||
GIT_REPOSITORY https://github.com/kokkos/kokkos.git | ||
GIT_TAG 4.0.01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not the latest release? Did you intend to update in a separate pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought a separate PR made more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How common do we expect it is that users will have their IDE setup to (remotely?) target a GPU for the tutorials?
@@ -40,7 +40,8 @@ int main(int argc, char *argv[]) { | |||
n, KOKKOS_LAMBDA(int i) { view(i) = 1 + i / 10.; }); | |||
|
|||
double result; | |||
Kokkos::parallel_reduce(n, GeometricMean{view}, result); | |||
// EXERCISE uncomment the following line when GeometricMean is implemented | |||
// Kokkos::parallel_reduce(n, GeometricMean{view}, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here, please propose this change as its own PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was necessary in order to compile this exercise. I will comment out the add_subdirectory
and do a separate PR.
As recommended by reviewers.
As recommended by reviewers.
As recommended by reviewers.
Kokkos_DEVICES is not set when using fetched version of Kokkos. As recommended by reviewers.
Warn if an accelerator is enabled. As recommended by reviewers.
As recommended by reviewers.
I cannot put any number, but laptops with decent GPUs are getting more popular, so I think compiling and trying the exercises on the local computer is natural. |
Cédric - a few general comments before diving into an 89-file review:
|
It would have. However, I do not think it is this useful to do it now that @dalg24 and @masterleinad have spent to much time browsing many files (sorry ...).
That's a great idea. I will add it in the README, but in another PR. |
Excellent. The more we can standardize and "automate" the tutorial/education process (e.g., even in ChatGPT) the greater the success we'll have. |
It does not make sense to split it and only tackle one example, regardless of how much time we spent on this before. |
|
This PR is about providing a parent project that includes all tutorials, and applying to all of them is more or less applying the same changes multiple places. Feel free to speak up if you don't like the general direction of this PR, but, so you know, I would strongly object to not tackling all (or least most) in this PR and I read Cedric's polite objection as him agreeing with me on that matter. |
if (Kokkos_ENABLE_CUDA OR Kokkos_ENABLE_HIP OR Kokkos_ENABLE_SYCL OR Kokkos_ENABLE_OPENMPTARGET OR Kokkos_ENABLE_HPX) | ||
message(WARNING "${CMAKE_CURRENT_SOURCE_DIR}" | ||
"A Kokkos accelerator backend is enabled, it might cause issue with the current program" | ||
"Please recompile with only a host backend enabled (e.g. -DKokkos_ENABLE_OPENMP=ON)") | ||
endif () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be moved into a separate function instead of repeating it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like a kokkos_tutorials_warn_cpu_only_program()
function or whatnot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that each exercise can be used as a standalone. I think factorizing with a function or macro makes the example less clear, and I wish I could avoid including a separate (but shared) file at the terminal CMakeLists.txt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It some way it is akin to the weird find kokkos module isn't it? I'd slightly prefer reusing code as Jakob suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had trouble finding the proper solution as the Kokkos_DEVICES
variable is not apparent. If I do a function, I will hide the details that might be useful to copy and paste for some users. That's the reason why it is better to be transparent there.
There are only two such exercises; we will probably not have to touch the CMakeLists.txt too often, so the function is not worth it.
However, I can add a comment explaining what this conditional does for better logic readability (which is the function's main advantage for me).
if (Kokkos_ENABLE_CUDA OR Kokkos_ENABLE_HIP OR Kokkos_ENABLE_SYCL OR Kokkos_ENABLE_OPENMPTARGET OR Kokkos_ENABLE_HPX) | ||
message(WARNING "${CMAKE_CURRENT_SOURCE_DIR}" | ||
"A Kokkos accelerator backend is enabled, it might cause issue with the current program" | ||
"Please recompile with only a host backend enabled (e.g. -DKokkos_ENABLE_OPENMP=ON)") | ||
endif () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like a kokkos_tutorials_warn_cpu_only_program()
function or whatnot?
How about just renaming the targets
The warning message for the device backends could go into the But this still requires us to adapt the slides for the tutorials and the only gain is if someone wants to build all exercises at the same time. |
The idea is to make sure that the installed Kokkos is used. The cmake "configuration" phase will fail if it is not correctly found. It is also helpful on the air-gapped systems to avoid hanging on `fetch_content.`
KokkosTutorials_KOKKOS_SOURCE_DIR can be used to point to Kokkos source and avoid downloading.
The goal is to provide a joint built for all exercises (and their solutions).
This way, compilation is straightforward for IDE users.
Currently, most of the exercises are built using the top-level CMake.
However, not all builds make sense, as CUDA builds are wrong for the first exercises. I have put configuration warnings when Kokkos is not configured correctly, as explaining why it is not working is still valuable.
I have also refactored the
fetch_content
implementation: it is now called in an overloadedfind_package
to keepCMakeLists.txt
as standard as possible.Three CMake variables can be set by the user to control the build system:
KokkosTutorials_FORCE_INTERNAL_Kokkos
: to force a build of a custom Kokkos and ignore any installed KokkosKokkosTutorials_FORCE_EXTERNAL_Kokkos
: to force finding an installed Kokkos and to fail if not foundKokkosTutorials_KOKKOS_SOURCE_PATH
: to specify a Kokkos source directory and by-passfetch_content
when building a custom Kokkos.I can improve the global CMake by usingexternal_project
for the first exercises to "sandbox" their builds and allow a custom Kokkos.