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

Tpetra: Simplify initialization paths #194

Closed
mhoemmen opened this issue Mar 8, 2016 · 7 comments
Closed

Tpetra: Simplify initialization paths #194

mhoemmen opened this issue Mar 8, 2016 · 7 comments
Assignees

Comments

@mhoemmen
Copy link
Contributor

mhoemmen commented Mar 8, 2016

THIS COMMENT SUPERSEDED BY MY LONG COMMENT BELOW (01:23 09 Mar 2016).

@trilinos/tpetra
Moved from Bugzilla Bug 6361: https://software.sandia.gov/bugzilla/show_bug.cgi?id=6361

That bug discussion centered around whether Tpetra should initialize Kokkos and MPI (if enabled) for users automatically. The motivating problem is that users encountered unexpected, undesired Kokkos behavior, because Tpetra initialized Kokkos with defaults, when it should instead have used Kokkos' input command-line arguments. The main benefit of automatic initialization is that Tpetra users who don't otherwise use Kokkos or MPI, don't have to know about either. The risks of automatic initialization are:

  1. Need to pass in command-line arguments through (argc, argv), for both Kokkos and MPI
  2. If we silently initialize Kokkos with defaults, users might get undesired behavior

We didn't all come to full agreement, but I think we can agree that the following two options make sense:

  • initialize(argc,argv): Tpetra initializes MPI (if not already) and Kokkos (if not already), passing in command-line arguments to both MPI_Init and Kokkos::initialize. Note that MPI_Init reserves the right to modify both argc and argv (e.g., to delete MPI's command-line arguments).
  • initialize(): Tpetra requires that both MPI and Kokkos are initialized. If not, the function throws.

It is unnecessary to provide an initialize() function that takes an MPI_Comm, because Tpetra does not have a "default MPI communicator." Tpetra::Map takes and wraps the user's communicator. Similarly, initialize() need not take a Kokkos execution space instance; the proper place for that would be the constructor of Tpetra::Map.

Note that the zero-argument version of initialize() above changes current behavior with respect to Kokkos initialization. Current behavior is that initialize() initializes Kokkos with default arguments.

@maherou
Copy link
Member

maherou commented Mar 8, 2016

I hope we are still on track to provide an embedded option for this feature. As was thoroughly discussed in the mentioned Bugzilla bug 6361, global initializers are bad for embedded libraries.

Can't we have an initializer object (instead of a global function) that behaves the same way, and then pass it in as an argument to Tpetra classes? That way we can have more than one kind of initializer in a code and can support Tpetra objects that, for example, exist on the host, while others exist on the device, or any other meaningful combination.

Mike

On Mar 8, 2016, at 7:32 AM, Mark Hoemmen [email protected] wrote:

@trilinos/tpetra
Moved from Bugzilla Bug 6361: https://software.sandia.gov/bugzilla/show_bug.cgi?id=6361

In summary: we might like to give the Tpetra::initialize function the following behavior:

initialize(): Tpetra should check whether Kokkos and MPI are initialized, and if not, should do it with their respective defaults
initialize(argc,argv): Tpetra should initialize MPI (if not already) and Kokkos (if not already), passing in command-line arguments
initialize(MPI_Comm, {Kokkos_ExecutionSpace_Instance}): just use what was initialized before, and if MPI or Kokkos was not initialized throw
This imitates Kokkos:

Kokkos::initialize() => Dear Kokkos figure out what to do
Kokkos::initialize(argc,argv) => use command line arguments to override automatic behavior. If argc is 0 (or no command line arguments recognized by kokkos are provided), it's the same as the version without arguments.
Kokkos::initialize(InitArguments) => we have a struct which lets you set the parameters. This is in particular useful if you have your own command line parsing mechanism, or everything is coming in via input files so you can't use Kokkos command line arguments.
However, please refer to the above Bugzilla Bug for discussion.


Reply to this email directly or view it on GitHub.

@mhoemmen mhoemmen self-assigned this Mar 9, 2016
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Mar 9, 2016

I hope we are still on track to provide an embedded option for this feature. As was thoroughly discussed in the mentioned Bugzilla bug 6361, global initializers are bad for embedded libraries.

Tpetra does not actually need initialization if Kokkos and MPI are initialized already. This is the embedded option. Tpetra::initialize() is a no-op in that case, and doesn't even need to exist (so let's get rid of it?).

@mhoemmen
Copy link
Contributor Author

mhoemmen commented Mar 9, 2016

Can't we have an initializer object (instead of a global function) that behaves the same way, and then pass it in as an argument to Tpetra classes?

That's what Map is :-) It takes an MPI communicator. In the future, we may extend it to take a Kokkos execution space instance as well.

@mhoemmen
Copy link
Contributor Author

mhoemmen commented Mar 9, 2016

To clarify further: the main point of this is to avoid unpleasant user surprise at Kokkos getting initialized with the wrong defaults. Tpetra already supports use as an embedded library. There are occasions when it initializes a global MPI_Op, for example, but it is careful to tie finalization of the MPI_Op to an MPI_Finalize hook. The only issue here is whether we even want Tpetra to expose to users an option to support Kokkos and MPI initialization. We're going to need that option for unit tests regardless.

@mhoemmen
Copy link
Contributor Author

mhoemmen commented Mar 9, 2016

Revised discussion:

@trilinos/tpetra
Moved from Bugzilla Bug 6361: https://software.sandia.gov/bugzilla/show_bug.cgi?id=6361

That bug discussion centered around whether Tpetra should initialize Kokkos and MPI (if enabled) for users automatically. In particular, the constructors of the "Node" objects that Tpetra uses (a legacy of Kokkos 1.0, retained only for backwards compatibility) initialize Kokkos if it hasn't already been initialized. (Tpetra::Map's constructor creates the Node automatically for them. Users aren't supposed to handle Node instances anymore, because we plan to phase them out.)

This is a problem because correct initialization of both Kokkos and MPI requires command-line arguments (argc, argv). There is currently no way for users to pass command-line arguments into Node's constructor or Map's constructor. This was never a problem for MPI, because Tpetra always required that MPI be initialized; Tpetra never tried to initialize MPI for users. The issue with Kokkos is that users never had to initialize Kokkos 1.0, and we didn't want to have to change their code.

It's easy to set up Kokkos and MPI initialization in Teuchos' unit test framework. If a Trilinos unit test executable buys into Teuchos' predefined main() function by linking with packages/teuchos/comm/test/UnitTesting/Teuchos_StandardParallelUnitTestMain.cpp, that main() function initializes MPI before any tests in the executable run, and finalizes MPI after all tests in the executable run. It does so using Teuchos::GlobalMPISession (RAII wrapper around MPI_Init / MPI_Finalize).

The weird thing is that Teuchos::GlobalMPISession's destructor finalizes Kokkos as well as MPI! Thus, wouldn't it be rational for Teuchos::GlobalMPISession's constructor (which conveniently takes (argc, argv)) to call Kokkos::initialize?

In summary: We could eliminate Tpetra::initialize (all overloads). For unit tests, Teuchos::GlobalMPISession can start Kokkos. For users' code, we expect users either to initialize Kokkos as boilerplate, or to use Teuchos::GlobalMPISession (or some "Tpetra::" equivalent). How about this?

@mhoemmen
Copy link
Contributor Author

My fixes for #432 and #434 resolve this issue. Please feel free to reopen, create a new issue, or post comments (whichever is appropriate), if you wish to continue discussion.

@mhoemmen
Copy link
Contributor Author

See also #291 (fixed). Tpetra Node initialization reads and parses the command-line arguments saved by Teuchos::GlobalMPISession, so it gets the command-line arguments correctly for all unit tests that use Teuchos' unit test "main" .cpp file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants