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

KokkosTools: Don't call callbacks before backends are initialized #6114

Merged
merged 4 commits into from
May 24, 2023
Merged

KokkosTools: Don't call callbacks before backends are initialized #6114

merged 4 commits into from
May 24, 2023

Conversation

masterleinad
Copy link
Contributor

Fixes the issue seen in kokkos/kokkos-tools#188 (comment).
In short, the test there sets callbacks before Kokkos is initialized and some callbacks are used when initializing backends before the respective tool is initialized. This pull request proposes to pause tools until the backends are initialized which also implies that none of those events get recorded.

@masterleinad masterleinad force-pushed the kokkos_tools_initialization_callbacks branch from 975ee2a to 1ac17c1 Compare May 9, 2023 14:50
@masterleinad masterleinad marked this pull request as ready for review May 9, 2023 15:49
@dalg24
Copy link
Member

dalg24 commented May 9, 2023

Please discuss why this is not just a precondition violation. Why we should support setting a callback before initialization.

@masterleinad
Copy link
Contributor Author

Please discuss why this is not just a precondition violation. Why we should support setting a callback before initialization.

We need to set the initializer callback before calling KokkosTools::initialize anyway. We could restrict to just this one callback (or all the ones that actually test true in the test directly after initialization) but that might not be practical also looking at https://github.com/kokkos/kokkos-tools/blob/7ab29004cf1d9a05328b704f0717ee686f1b095b/example/main.cpp#L12-L38. It seems sensible to be able to say

auto eventSet = SpaceTimeStack::get_event_set();
Kokkos::Tools::Experimental::set_callbacks(eventSet);

when the tools already provide a get_event_set function (in a tool-specific namespace).

@masterleinad masterleinad added this to the Release 4.1 milestone May 9, 2023
@dalg24
Copy link
Member

dalg24 commented May 9, 2023

We need to set the initializer callback before calling KokkosTools::initialize anyway.

I do not understand.

@masterleinad
Copy link
Contributor Author

I do not understand.

The tools provide an init_library callback that is used to initialize the tool, see, e.g., https://github.com/kokkos/kokkos-tools/blob/7ab29004cf1d9a05328b704f0717ee686f1b095b/profiling/space-time-stack/kp_space_time_stack.cpp#L802-L806. This callback must be called before any other callback the tool provides and this is (only) done in the call to Kokkos::Profiling::initialize which is part of Kokkos::initialize. Thus, the (init_library) callback must be set before calling Kokkos::initialize to have any effect.

Rombur
Rombur reviewed May 15, 2023
View reviewed changes
core/unit_test/tools/TestToolsInitialization.cpp Outdated
Kokkos::deep_copy(dst_view, src_view);
Kokkos::parallel_for("parallel_for",
Kokkos::RangePolicy<execution_space>(0, 1),
[=](int i) { (void)i; });
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
[=](int i) { (void)i; });
KOKKOS_LAMBDA(int i) { (void)i; });

core/unit_test/tools/TestToolsInitialization.cpp Outdated Show resolved Hide resolved
crtrott
crtrott reviewed May 18, 2023
View reviewed changes
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Proposed cleaner comment

core/src/impl/Kokkos_Core.cpp Outdated Show resolved Hide resolved
@masterleinad @crtrott
Improve comment
e13d8a0 
Co-authored-by: Christian Trott <crtrott@sandia.gov>
@masterleinad
Copy link
Contributor Author

Retest this please.

@masterleinad
Fix: The enclosing parent function ("TestBody") for an extended __hos…
ec79d9c 
…t__ __device__ lambda cannot have private or protected access within its class
@masterleinad masterleinad force-pushed the kokkos_tools_initialization_callbacks branch from 3ce5887 to ec79d9c Compare May 19, 2023 18:42
@masterleinad
Copy link
Contributor Author

Retest this please.

@dalg24 dalg24 merged commit 12e9645 into kokkos:develop May 24, 2023
28 checks passed
@dalg24
Copy link
Member

dalg24 commented May 24, 2023

Plz make sure you add to the changelog

@masterleinad masterleinad mentioned this pull request May 25, 2023
Closed
@romintomasetti romintomasetti mentioned this pull request May 31, 2023
Merged
2 tasks
nliber pushed a commit to nliber/kokkos that referenced this pull request Jun 22, 2023
@masterleinad @crtrott @nliber
KokkosTools: Don't call callbacks before backends are initialized (ko…
ed2ec96 
…kkos#6114)

* KokkosTools: Don't call callbacks before backends are initialized

* [=]->KOKKOS_LAMBDA

* Improve comment

Co-authored-by: Christian Trott <crtrott@sandia.gov>

* Fix: The enclosing parent function ("TestBody") for an extended __host__ __device__ lambda cannot have private or protected access within its class

---------

Co-authored-by: Christian Trott <crtrott@sandia.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rombur Rombur Rombur approved these changes

@crtrott crtrott crtrott left review comments

@dalg24 dalg24 dalg24 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 4.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@masterleinad @dalg24 @Rombur @crtrott