kokkos-tools space-time-stack: allow API access to the stack

Summary

This PR:

~~Adds Google Test from a git tag iif tests are enabled~~ Done in #237
Moves declarations from kp_space_time_stack.cpp to kp_space_time_stack.hpp to enable users make API-driven requests to the State
Switches to std::chrono::steady_clock for recording the duration of the frames as generally recommended

Description

Note for the reviewers: if possible, review the changes in an editor that "discards" lines whose sole change is whitespace, this really helps :wink: (e.g. VS Code)

Google Test was needed for better testing. See for instance #195.

The need for moving declarations to a dedicated header file is driven by:

foster reuse (e.g. StackNode is also used in kp_chrome_tracing.cpp
enable users to make API requests

The second point is worth explaining in more details.

In fact, we think that space-time-stack could be a replacement for Teuchos::TimeMonitor. Indeed, the approach with Teuchos timers is generally to enable them via a compile definition (HAVE_TPETRA_MMM_TIMINGS for instance) or using some sort of parameter. These approaches are not easy:

compile time means you need to recompile a whole library to enable the timers (they are usually off by default)
parameters need to be passed all around (or kept in some sort of static manager) so it's intrusive

Moreover Teuchos timers are not easily made "compatible" with Kokkos Tools callbacks.

Having API access to space-time-stack helps in that:

the library need not be recompiled since by default callbacks are nullptr but can be set via command line
no need for parameters since callbacks act like parameters (on/off driven by nullptr or not)

So a user that today uses Teuchos timers can simply change with Kokkos profile regions and scoped regions and enjoy a unified approach, and can make queries to the State, e.g. during a benchmark in which manual timers are used.

TEST(SpaceTimeStack, demangle) {
  //! Initialize @c Kokkos.
  Kokkos::initialize(argc, argv);

  //! Redirect output for later analysis.
  std::cout.flush();
  std::ostringstream output;
  std::streambuf* coutbuf = std::cout.rdbuf(output.rdbuf());

  //! Run tests. @todo Replace this with Google Test.
  Tester tester(Kokkos::DefaultExecutionSpace{});

  //! Finalize @c Kokkos.
  Kokkos::finalize();

  //! Restore output buffer.
  <here comes the test assertions using Google Test>
}

So basically we would never initialize Kokkos in the main, only Google Test would be. I think it's OK if you have very few tests and a "lightweight" backend like OpenMP but I guess it is more of an issue when Kokkos initializes GPU backends... Not sure it would be a good idea to initialze/finalize many times in this case.

Feb 06 '24 17:02 romintomasetti

@masterleinad Let's move the discussion to #237 :wink: (wherein only Google Test is added)

Feb 06 '24 19:02 romintomasetti

The existing test looks at the output of the space-time-stack tool when it is "destroyed", i.e. when Kokkos gets finalized.

Currently, it works like this:
initialize Kokkos



redirect std::cout to std::ostringstream



launch kernels



fnialize kokkos



look at what we received in the buffer
So I guess it is not easy to make the current test use something like you're mentioning (https://github.com/kokkos/kokkos/blob/master/core/unit_test/UnitTestMainInit.cpp).

As an alternative (that I don't really like), we could do:
TEST(SpaceTimeStack, demangle) {

  //! Initialize @c Kokkos.

  Kokkos::initialize(argc, argv);



  //! Redirect output for later analysis.

  std::cout.flush();

  std::ostringstream output;

  std::streambuf* coutbuf = std::cout.rdbuf(output.rdbuf());



  //! Run tests. @todo Replace this with Google Test.

  Tester tester(Kokkos::DefaultExecutionSpace{});



  //! Finalize @c Kokkos.

  Kokkos::finalize();



  //! Restore output buffer.

  <here comes the test assertions using Google Test>

}
So basically we would never initialize Kokkos in the main, only Google Test would be. I think it's OK if you have very few tests and a "lightweight" backend like OpenMP but I guess it is more of an issue when Kokkos initializes GPU backends... Not sure it would be a good idea to initialze/finalize many times in this case.

@masterleinad Is this good with you?

Feb 06 '24 20:02 vlkale

@vlkale @masterleinad I added the test_State.cpp test. This test is really what I had in mind:

Enable in-depth testing without waiting for the tool to finalize. It means you don't need to check the output of the tool, but you can directly query it.
Allow users to access the state.

Mar 29 '24 15:03 romintomasetti

@vlkale @masterleinad Have you had a chance to look at this yet ? :smile:

Apr 24 '24 08:04 romintomasetti

@romintomasetti

Thanks for your work on this. I have a couple of quick comments.

Is there a github Issue associated with this PR? Can you bring forth a use case for this?
I think the PR could still be broken into smaller components. I know you argued against that, but I don't know if @masterleinad is OK with that?
I am specifically wondering how we can replicate/use the Google Tests part of this PR (and other Google Test / CTests currently in develop) in the third-party vendor connectors, e.g., nvtx-connector, nvtx-focused-connector. My opinion: testing that thoroughly (even though simple) has a little more priority. I know that we are testing tools individually, but I think it may be good to think of Google Tests / CTest efforts in a unified way, at least for the vendor-connectors.

Apr 24 '24 18:04 vlkale

@vlkale

1. Is there a github Issue associated with this PR? Can you bring forth a use case for this?

The use case is described in details in the PR description. In summary, allow for code reuse and allow user to make API requests to the space-time-stack tool.

2. I think the PR could still be broken into smaller components. I know you argued against that, but I don't know if @masterleinad is OK with that?

Not sure I understand the point. I've extracted the Google Test related stuff from this PR to #237. This PR is now only concerned with modifying and testing the space-time-stack tool, see PR description.

3. I am specifically wondering how we can replicate/use the Google Tests part of this PR (and other Google Test / CTests currently in develop) in the third-party vendor connectors, e.g., nvtx-connector, nvtx-focused-connector. My opinion: testing that thoroughly (even though simple) has a little more priority. I know that we are testing tools individually, but I think it may be good to think of Google Tests / CTest efforts in a unified way, at least for the vendor-connectors.

I guess thinking of how the vendor connectors can be tested is a bigger picture question. As a reminder, the CICD of this repo still runs on GitHub runners. So testing vendor connectors for GPUs is not even possible for now.

I would also propose that this PR does not get blocked by such a bigger picture question.

Apr 25 '24 13:04 romintomasetti

@romintomasetti Thanks for your replies.

Replies inline.

To summarize:

I think putting the text for the use case in a new GIthub Issue ought to be done (this does not solve #191 as I understand).
it is not my intention for this PR to handle the higher-level / out-of-scope questions, but I am intentionally being high-level to try to prioritize this PR on the Kokkos Tools planning board.

@vlkale
1. Is there a github Issue associated with this PR? Can you bring forth a use case for this?
The use case is described in details in the PR description. In summary, allow for code reuse and allow user to make API requests to the space-time-stack tool.

OK, thanks for that. I think the reason to do that is so that Kokkos Tools users and developers can more easily test space-time-stack. In any case, I think the second sentence is a good opening of a new Github Issue. As per Kokkos Tools development convention (I want to create a contributing.md), we should write this and then put at the top of this PR 'Fix Github Issue # xyz').

The PR #237 was a good one that addressed the testing github issue well (https://github.com/kokkos/kokkos-tools/issues/191). This PR has some motivation for it and is related, but this is separate from that. At least, when I wrote that Github Issue, this PR was not my intention.

2. I think the PR could still be broken into smaller components. I know you argued against that, but I don't know if @masterleinad is OK with that?
Not sure I understand the point. I've extracted the Google Test related stuff from this PR to #237. This PR is now only concerned with modifying and testing the space-time-stack tool, see PR description.
3. I am specifically wondering how we can replicate/use the Google Tests part of this PR (and other Google Test / CTests currently in develop) in the third-party vendor connectors, e.g., nvtx-connector, nvtx-focused-connector. My opinion: testing that thoroughly (even though simple) has a little more priority. I know that we are testing tools individually, but I think it may be good to think of Google Tests / CTest efforts in a unified way, at least for the vendor-connectors.
I guess thinking of how the vendor connectors can be tested is a bigger picture question. As a reminder, the CICD of this repo still runs on GitHub runners. So testing vendor connectors for GPUs is not even possible for now.

OK. I would still say this is a priority given user's often go to these connectors, but this is not to demote priority of space-time-stack, as it is one of the most useful non-vendor-connectors to users. And yes, you are right on the CICD github runners.

That said, let me suggest something more focused to this PR: Should all tests of space-time-stack developed be required to run on the Github runners? Do we provide some tests for space-time-stack as-is in Kokkos Tools, which are only applicable and pertinent to GPUs? I think your allowing the API access to the stack addresses this problem, but maybe there is a different solution to the problem.

I would also propose that this PR does not get blocked by such a bigger picture question.

OK. We can leave nvtx-connector out. I would assume the changes you propose should be considered for other connectors. It's not my intention to block this PR and I see after thinking about it that we don't need to.

Apr 29 '24 18:04 vlkale

space-time-stack: allow API access to the stack

Summary

Description

Related