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

[romintomasetti]

Summary

This PR:

• Adds Google Test from a git tag iif tests are enabled Done in tests: add Google Test and use it to test space-time-stack connector #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 😉 (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:

1. foster reuse (e.g. StackNode is also used in kp_chrome_tracing.cpp
2. 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.

Related

• as discussed in tool(space-time-stack): demangle name #218

[romintomasetti]

@maartenarnst

[romintomasetti]

@vlkale @dalg24 @masterleinad Would you have some time to give your feedback on this one ? Thanks 🙏

[masterleinad]

You are combining two independent ideas. Please split them or provide some justification as to why this is not possible.

[romintomasetti]

@masterleinad I could indeed separate them. I could first introduce Google Test.

But I need the "moved to header file" feature to effectively use Google Test in test_demangling.cpp.

Is there any way I can load the space-time-stack tool in a test setup and unload it in the test tear down ?

[romintomasetti]

@masterleinad If not, you've got 2 options IMO:

1. We make a PR that introduces Google Test, but it won't be used anywhere. Then we proceed with this PR.
2. We introduce both now.

Your choice 😉

[masterleinad]

We make a PR that introduces Google Test, but it won't be used anywhere. Then we proceed with this PR.

Why can't you just convert the existing test (and mimic the way we use UnitTestMainInit in https://github.com/kokkos/kokkos)?

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

[romintomasetti]

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

[vlkale]

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?

[romintomasetti]

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

[romintomasetti]

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

[vlkale]

@romintomasetti

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

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

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?

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.

[romintomasetti]

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

[dalg24]

I really don't like exposing the internals of the tool for the sake of testing it.
In the end we still need some testing of the actual output.

Why did you do chose this approach over running a dummy program and asserting that the right things get printed to the screen?

I expect it is fairly easy for regions. You could use std::this_thread::sleep_for if you want some control over their ballpark duration.

As for kernels, you can check that short running that are below the threshold do not show. Things like that.

[dalg24]

How about you declare it as last enumerator in Space?

[dalg24]

(to be clear I don't think you should change anything in the tool, In think you should consider a different approach for testing)

[dalg24]

I looked over the implementation and can't help asking what is the benefit of the switch case over a simple

char *space_names[] = {
  "HOST",
  "CUDA",
  ...
};

especially now that you pin their value in the enumeration declaration?
(not asking you to change here)

[masterleinad]

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.

What is actually missing to replace Teuchos::TimeMonitor with some Kokkos Tools? I don't think that exposing State to Trilinos users is a good idea. I could see that it could make sense to expose it for a different tool to use it, though. Or you would just add missing functionality to SpaceTimeStack directly.

[masterleinad]

Why do you change this test? The common use case is to use KOKKOS_TOOLS_LIBS.

[vlkale]

@romintomasetti I think we want to maintain the test from #237, which uses KOKKOS_TOOLS_LIBS.

If you want to do this, maybe we put back the original Google TEST (that doesn't use the new feature from this PR) in this .cpp file?

Or, maybe this file should not actually not be touched for this PR and can put the TEST fixture (TEST_F) in another .cpp file?

[vlkale]

@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 Create unit tests for all Kokkos Tools libraries and utilities #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 (#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.