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

Global CMake for tutorials #91

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

cedricchevalier19
Copy link
Member

@cedricchevalier19 cedricchevalier19 commented May 13, 2024

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 overloaded find_package to keep CMakeLists.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 Kokkos
  • KokkosTutorials_FORCE_EXTERNAL_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-pass fetch_content when 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.

@cedricchevalier19 cedricchevalier19 mentioned this pull request Jun 5, 2024
4 tasks
@cedricchevalier19
Copy link
Member Author

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.

@cedricchevalier19 cedricchevalier19 self-assigned this Jun 5, 2024
@cedricchevalier19 cedricchevalier19 marked this pull request as ready for review June 10, 2024 14:18
@cedricchevalier19
Copy link
Member Author

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.

Exercises/.cmake/FindKokkos.cmake Show resolved Hide resolved
Exercises/.cmake/FindKokkos.cmake Show resolved Hide resolved
FetchContent_Declare(
Kokkos
GIT_REPOSITORY https://github.com/kokkos/kokkos.git
GIT_TAG 4.0.01
Copy link
Contributor

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?

Copy link
Member Author

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.

Exercises/01/Begin/CMakeLists.txt Outdated Show resolved Hide resolved
Exercises/01/Begin/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@dalg24 dalg24 left a 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?

CMakeLists.txt Outdated Show resolved Hide resolved
Exercises/01/Begin/CMakeLists.txt Outdated Show resolved Hide resolved
Exercises/01/Begin/CMakeLists.txt Show resolved Hide resolved
Exercises/01/Begin/CMakeLists.txt Outdated Show resolved Hide resolved
Exercises/instances/Solution/instances_solution.cpp Outdated Show resolved Hide resolved
@@ -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);
Copy link
Member

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.

Copy link
Member Author

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.

Exercises/tools_minimd/CMakeLists.txt Outdated Show resolved Hide resolved
Exercises/multi_gpu_cuda/Begin/CMakeLists.txt Outdated Show resolved Hide resolved
Exercises/multi_gpu_cuda/Solution/CMakeLists.txt Outdated Show resolved Hide resolved
Exercises/unique_token/Begin/CMakeLists.txt Show resolved Hide resolved
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.
@cedricchevalier19
Copy link
Member Author

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.

@ajpowelsnl
Copy link
Contributor

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

@cedricchevalier19
Copy link
Member Author

  • 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.

@ajpowelsnl
Copy link
Contributor

ajpowelsnl commented Jun 12, 2024

  • 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.

@dalg24
Copy link
Member

dalg24 commented Jun 12, 2024

  • 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.

@ajpowelsnl
Copy link
Contributor

  • 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

@dalg24
Copy link
Member

dalg24 commented Jun 12, 2024

  • 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.

Comment on lines +9 to +13
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 ()
Copy link

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?

Copy link
Member

@dalg24 dalg24 Jun 13, 2024

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?

Copy link
Member Author

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.

Copy link
Member

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

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 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).

Comment on lines +9 to +13
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 ()
Copy link
Member

@dalg24 dalg24 Jun 13, 2024

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?

Exercises/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

just out of curiosity why "dot"cmake for the directory name?

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 notoriously bad at naming things. Initially, I wanted to put the helper files in a classic cmake directory, but having such a utility directory in the middle of the exercise was confusing. So, I thought it was clever to hide it with a starting dot.
Any better naming scheme is welcomed.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer w/o the dot

Exercises/CMakeLists.txt Outdated Show resolved Hide resolved
@JBludau
Copy link

JBludau commented Jun 13, 2024

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.

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.
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.

7 participants