Global CMake for tutorials
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_contentimplementation: ~it is now called in an overloadedfind_packageto keepCMakeLists.txtas standard as possible~.
The user can set three CMake variables to control the build system:
- CMAKE_DISABLE_FIND_PACKAGE_Kokkos: to force a build of a custom Kokkos and ignore any installed Kokkos
- CMAKE_REQUIRE_FIND_PACKAGE_Kokkos: to force finding an installed Kokkos and to fail if not found
KokkosTutorials_KOKKOS_SOURCE_PATH: to specify a Kokkos source directory and by-passfetch_contentwhen building a custom Kokkos.
~I can improve the global CMake by using external_project for the first exercises to "sandbox" their builds and allow a custom Kokkos.~
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.
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.
How common do we expect it is that users will have their IDE setup to (remotely?) target a GPU for the tutorials?
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:
- Would it make sense to radically narrow this PR, and convert one of the exercises to CMake to make your proposed changes easier to understand? We just need one strong proof-of-concept example to understand how we could / should extend to the other exercises
- With the one CMake-i-fied exercise example, demonstrate correct linking and runtime behavior for the target backend
- It's likely our shortcoming that we don't specify in the tutorials how students should build Kokkos. Would it make sense to point students to the Quick Start at the start of the tutorial for a semi-standard Kokkos configure / build / install
- Would it make sense to radically narrow this PR, and convert one of the exercises to CMake to make your proposed changes easier to understand? We just need one strong proof-of-concept example to understand how we could / should extend to the other exercises
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 ...).
- It's likely our shortcoming that we don't specify in the tutorials how students should build Kokkos. Would it make sense to point students to the Quick Start at the start of the tutorial for a semi-standard Kokkos configure / build / install
That's a great idea. I will add it in the README, but in another PR.
- Would it make sense to radically narrow this PR, and convert one of the exercises to CMake to make your proposed changes easier to understand? We just need one strong proof-of-concept example to understand how we could / should extend to the other exercises
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 ...).
- Let us not succumb to the "sunk costs" fallacy π ; the fact that others have begun reviewing the unwieldy PR is not a real argument against doing one simple, clear, correct example. Would you consider closing this PR, and using what you learned here to create one proof-of-concept example?
- It's likely our shortcoming that we don't specify in the tutorials how students should build Kokkos. Would it make sense to point students to the Quick Start at the start of the tutorial for a semi-standard Kokkos configure / build / install
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.
- Would it make sense to radically narrow this PR, and convert one of the exercises to CMake to make your proposed changes easier to understand? We just need one strong proof-of-concept example to understand how we could / should extend to the other exercises
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 ...).
- Let us not succumb to the "sunk costs" fallacy π ; the fact that others have begun reviewing the unwieldy PR is not a real argument against doing one simple, clear, correct example. Would you consider closing this PR, and using what you learned here to create one proof-of-concept example?
It does not make sense to split it and only tackle one example, regardless of how much time we spent on this before.
- Would it make sense to radically narrow this PR, and convert one of the exercises to CMake to make your proposed changes easier to understand? We just need one strong proof-of-concept example to understand how we could / should extend to the other exercises
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 ...).
- Let us not succumb to the "sunk costs" fallacy π ; the fact that others have begun reviewing the unwieldy PR is not a real argument against doing one simple, clear, correct example. Would you consider closing this PR, and using what you learned here to create one proof-of-concept example?
It does not make sense to split it and only tackle one example, regardless of how much time we spent on this before.
- Separating the question of time from the discussion, if each exercise is stand alone, I don't understand why a number of moderately correct / incorrect examples would be more instructive (as a proof-of-concept) than one clear, more correct example
- Would it make sense to radically narrow this PR, and convert one of the exercises to CMake to make your proposed changes easier to understand? We just need one strong proof-of-concept example to understand how we could / should extend to the other exercises
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 ...).
- Let us not succumb to the "sunk costs" fallacy π ; the fact that others have begun reviewing the unwieldy PR is not a real argument against doing one simple, clear, correct example. Would you consider closing this PR, and using what you learned here to create one proof-of-concept example?
It does not make sense to split it and only tackle one example, regardless of how much time we spent on this before.
- Separating the question of time from the discussion, if each exercise is stand alone, I don't understand why a number of moderately correct / incorrect examples would be more instructive (as a proof-of-concept) than one clear, more correct example
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.
How about just renaming the targets 01_Exercise -> 01_Begin and 01_Solution, but leaving the build system as it is. For IDE users we can still provide a IDE/CMakeLists.txt file with something like:
cmake_minimum_required(VERSION 3.16)
project(KokkosTutorialAll)
add_subdirectory(../Exercises/01/Begin)
add_subdirectory(../Exercises/01/Solution)
...
The warning message for the device backends could go into the common.cmake.
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.
I think this was a good improvement for the maintenance of the tutorials. It's sad to see this PR stalled. Could the current improvements be incorporated, and the moot points be tackled down by another PR?
By the way, we used a very similar approach for the CExA Kokkos tutorials and it serves our needs just right.
The conflicts would need to be resolved. I would want to double check that when building individual exercises we do not re-download the source as it was the case last summer.
The current common.cmake forces FetchContent_Declare to use a source directory, so that it would not re-download it (this is actually an option passed to ExternalProject_Add). And it uses a directory in the source tree.
The new FindKokkos.cmake with the same option uses a directory in the build tree instead, which makes it always downloaded.
Note that the download directory can be set instead.
To re-use the already downloaded sources, we can either use a directory somewhere in the source tree that makes sense (and would be in .gitignore, this is kinda what we use in the CExA tutorial), or in the system temporary directory (by instance $ENV{TMP}/kokkos).
I will update against the current main branch and check how we download Kokkos.
@JBludau and @dalg24, I finally decided to follow your advice and I've factorized the code checking for a gpu. Furthermore, to apply the good practice described in the documentation to use either find_package and fetch_content, I have reverted to hide the magic behind a custom FindKokkos.cmake.
I can update the CI to use the new global CMake but I think it is better to leave it to a separate PR, this one being already too large.
I do not understand the issue with Windows but it looks similar to the one of #107.
So, creating a custom FindKokkos.cmake is not a good idea?
There is also a missing include:
diff --git a/Exercises/unique_token/Begin/unique_token.cpp b/Exercises/unique_token/Begin/unique_token.cpp
index 0af6bec..800a4f4 100644
--- a/Exercises/unique_token/Begin/unique_token.cpp
+++ b/Exercises/unique_token/Begin/unique_token.cpp
@@ -1,4 +1,5 @@
#include<Kokkos_Core.hpp>
+#include <iostream>
// EXERCISE: need to remove the ifdef...
#ifdef KOKKOS_ENABLE_OPENMP