Inconsistent default execution spaces between kokkos and tpetra

[rppawlo]

@trilinos/tpetra

We are having some failures in panzer that have been tracked back to inconsistent default execution spaces between tpetra and kokkos. Panzer uses the tpetra default default execution space, but panzer depends on intrepid2 which uses the default execution space from kokkos.

When a user does not specify the kokkos and tpetra execution spaces explicitly, there is a high probability that the execution spaces are mismatched. The Threads TPL usually is automatically enabled due to ctest examining the environment (if not set explicitly in configure). Kokkos will choose a pthread execution space if TPL_ENABLE_Pthread is on. Tpetra, though, sees both a serial and the pthread execution spaces available, but chooses serial instead of pthread:

# Don't turn on both Serial and Threads by default.  Prefer Serial,
# because Threads is a last resort.  Of course, users may still
# enable Threads explicitly, by setting TPETRA_INST_PTHREAD=ON.
IF (HAVE_TPETRA_INST_SERIAL_DEFAULT AND HAVE_TPETRA_INST_PTHREAD_DEFAULT)
  GLOBAL_SET(HAVE_TPETRA_INST_PTHREAD_DEFAULT OFF)
ENDIF ()

So what happens is that in trilinos ci testing, Panzer tests assume serial space from Tpetra default, but the intrepid2 kernels default to Threads and the tests fail because the threads space is not initialized by panzer tests.

All currently failing panzer tests are due to this. I tried switching panzer over to use the kokkos default node type, but then the tests fail to build because tpetra does not build the pthread node under ETI.

I know we can fix this by going into all our trilinos nightly testing configure scripts and explicitly specifying a consistent execution space, but I believe this is going to be an issue for Trilinos users in general. Many don't use kokkos and don't know to set the execution spaces and make sure that they are consistent across packages. Can we set the defaults consistently across packages (for Tpetra to directly key off of Kokkos), yet still allow users to override the default execution spaces in each package.

By the way - we have cases where application codes need both serial and pthread spaces. DTK only cares about MPI communication and is therefore hard coded to a Serial execution space in Tpetra (to avoid having to push templates through the entire stack). Albany uses DTK for multiphysics coupling but is also Kokkos aware and wants to use kokkos in a parallel execution space internally.

[crtrott]

The problem is that we have stuff which requires Pthreads on in Trilinos. So we can't turn off pthreads generally. What we could do is let Kokkos use the same logic as Tpetra. I.e. to turn pthreads support on in Kokkos you must set Kokkos_ENABLE_PTHREAD=ON. It will not be enough to just enable pthreads as a TPL as is currently done with OpenMP.

[rppawlo]

That would be great. It would fix the tests failures in panzer.

I'm curious to know why tpetra would rather default to serial over pthread (opposite of kokkos). Is that outdated information or still the case even with the kokkos refactor?

[crtrott]

The reason is that we more or less enabled pthreads by default in Trilinos, but most people still just want serial builds. Our philosophy was you should need to do something to enable threading in Solvers etc, in particular since that was not working stable yet. With OpenMP you need to do something in the configure script. The pthreads TPL was kinda on by default.

[mhoemmen]

Hi @rppawlo ! We could always change the Tpetra defaults. The reason Tpetra defaulted to Serial over Threads, was because Trilinos enabled the Pthreads TPL by default. This meant that users were getting Pthreads by default. These threads would fight with those in OpenMP-enabled libraries like Intel's MKL.

Other Tpetra customers tend to (over)specify things, so changing the Tpetra default to follow Kokkos probably won't break anything. What do you think, @crtrott ?

[crtrott]

I think we should change the logic of Kokkos in CMake. I.e. to actually get threads in Kokkos you have to set the Kokkos_ENABLE_PTHREAD variable on, and it will not default to the Trilinos setting for Pthreads.

[crtrott]

We could make the change simultaneously in the Trilinos and Kokkos repo, so we don't have to wait for the integration.

[rppawlo]

I'm happy with either path - just need them to be consistent.

Since panzer logic for default node mirrors current tpetra logic, if you choose to change the tpetra logic to match kokkos, it will break panzer default consistency. But I am happy to fix that when the push goes in. If you make kokkos mirror tpetra logic then we don't need to change panzer.

[rppawlo]

Thanks for working on this!

[crtrott]

Mark can you put a patch together apply it to the Trilinos repo, and send it to me in an email so I can apply it to the kokkos repo? You can also put a pull request for kokkos/develop out, but if you just send me a patch its fine as well.

[mhoemmen]

Hi Christian! I'll be happy to do that. Do we actually need to change Kokkos at the same time as Tpetra? Could Tpetra just take Kokkos' default execution space?

[mhoemmen]

Hi Christian! I'm looking at how Kokkos picks the default execution space. It looks like the logic for this is in kokkos/core/src/Kokkos_Macros.hpp, lines 396-441. The problem is that Tpetra currently has no way to query CMake variables to figure out Kokkos' default execution space. I could change Kokkos to fix this, but Kokkos needs to build without CMake. Thus, CMake alone shouldn't control Kokkos' choice of default execution space (unlike with Tpetra).

What I could do is this:

1. Add CMake logic to Kokkos, that imitates Kokkos' macro logic but doesn't control it
2. Add error checks to the macro logic to make sure that the CMake logic is consistent with it
3. Make Tpetra follow Kokkos' CMake logic for choice of default execution space

What do you think?

[rppawlo]

Hi @mhoemmen ! From what I gather, I think Christian prefers the default logic in tpetra over kokkos - where getting a pthread default node type requires active user intervention. So I think that is why he wants kokkos defaults changed rather than tpetra defaults. I like the path you itemized above. Since the default in kokkos is done in macro code, it would be great for the cmake in kokkos to do a configure time query to find the default node and set a global cmake variable that tpetra and panzer can key off of (item 22 in your list?). That way if the defaults change in the future, we don't have to change the downstream packages. I'm happy to help with this if needed.

[mhoemmen]

Thanks Roger! It's pretty late here, so I plan to work on this in the morning, but I would certainly welcome your help. I just don't want you to feel obligated!

On Feb 4, 2016, at 10:26 PM, rppawlo notifications@github.com<mailto:notifications@github.com> wrote:

Hi @mhoemmenhttps://github.com/mhoemmen ! From what I gather, I think Christian prefers the default logic in tpetra over kokkos - where getting a pthread default node type requires active user intervention. So I think that is why he wants kokkos defaults changed rather than tpetra defaults. I like the path you itemized above. Since the default in kokkos is done in macro code, it would be great for the cmake in kokkos to do a configure time query to find the default node and set a global cmake variable that tpetra and panzer can key off of (item 22 in your list?). That way if the defaults change in the future, we don't have to change the downstream packages. I'm happy to help with this if needed.

Reply to this email directly or view it on GitHubhttps://github.com/trilinos/Trilinos/issues/123#issuecomment-179868697.

[rppawlo]

No problem at all. I hope to work on this on Monday. Since I don't have the extensive testing infrastructure set up for tpetra and Kokkos, I will send some pull requests to you and Christian.

[etphipp]

I am wondering if it would make more sense to just eliminate pthreads from being a "tentatively enabled TPL" so it wouldn't get enabled by default. Frankly the tentatively enabled TPL idea was a bad idea from the beginning, and we continue to suffer the consequences. The problem I see with enabling pthreads in Kokkos only if the user explicitly enables it is that is different behavior than most other TPLs. For most (maybe all) TPLs, if a package optionally depends on it, it gets enabled by default in that package. You are basically providing an exception in this case, which I think would cause a lot of confusion for users.

[bmpersc]

I second Eric's suggestion for pthread and "tentatively enabled TPLs" in general.

[crtrott]

I thought someone has a required dependency on pthread. If that is not the case I would vote for: don't enable it by default, and we change Tpetra to just take Kokkos's default space.

[rppawlo]

That works great and is easier to implement. I'll test this out with panzer and put up a pull request soon.

[rppawlo]

Since this change impacts quite a bit of trilinos users, I sent out a user request to the developers list asking for input on this before we make a change to eliminate the "tentatively enabled TPLs".

[mhoemmen]

I thought someone has a required dependency on pthread.

STK Classic optionally depends on ThreadPool, which requires Pthreads. I think that's the only direct dependency, and it's optional.

[mhoemmen]

I agree that we should get rid of "tentatively enabled TPLs." Just because Trilinos detects Pthreads, doesn't mean that any package has to use it. We shouldn't abuse TriBITS to control what TPLs packages can use.

[rppawlo]

Summary: Trilinos usually only enables TPLs if the user sets TPLENABLE=ON. This is not the case for a handful of "tentatively enabled TPLs" of which Pthreads falls into. Each package can override and disable support for optional TPL dependencies but will default to ON if the TPL is enabled. This is how 99% of package dependencies operate. We want to be consistent with this logic.

What we propose to do is:

1. Change the pthreads TPL to act like a normal TPL - stop the "tentatively enabled" logic. This will prevent kokkos from defaulting to pthreads in traditional builds where a default node is not specified (much of Trilinos testing of older type 1 stack).
2. Set Tpetra configure logic to use the default node type that Kokkos has chosen. This will stop conflicts with different default spaces.

There is one complication with this. Since Kokkos does not use cmake/tribits exclusively, it chooses the default space in a macros header (Kokkos_Macros.hpp) based on preprocessor defines. If the build is using trilinos/tribits, the defines for explicitly picking a default space come from including the KokkosCore_config.hpp file in the Kokkos_Macros.hpp file. If no default is defined, the Kokkos_macros.hpp file picks the most logical execution space based on which execution spaces are enabled. To keep the logic consistent and implemented in only one place, cmake/tribits should do try_compile to interrogate the default space if no space is explicitly defined (in order to pass along cmake flag to tpetra cmake logic). But the try_compile command has to test each space separately and automatically reports failures to the screen for each try. This is probably confusing to users since they will see multiple interrogation failures during configure. Maybe there is a way to hide this reporting? The alternative is to duplicate the logic form the Kokkos_macros.hpp file in Kokkos cmake/tribits file. This is fragile in that any change to default node logic in Kokkos_Macros.hpp file needs to be duplicated here, but eliminates this confusing reporting during configure. Since we don't add execution spaces too often this is probably ok. Any opinions/suggestions here? The try_compile stuff also has to occur after the generation of the KokkosCore_config.hpp.

[bartlettroscoe]

I think this can be handled with some checks before and after the dependency logic runs. At the very least we can detect these inconsistency at configure time and fail hard before you get to a build or runtime issue. Then we should make the set of TPLs the set that we want to be turned on by default. If that set is zero, then that is fine with me.

[bartlettroscoe]

1. Change the pthreads TPL to act like a normal TPL - stop the "tentatively enabled" logic. This will prevent kokkos from defaulting to pthreads in traditional builds where a default node is not specified (much of Trilinos testing of older type 1 stack).

Is there any utility is keeping the tentative enable of Pthreads? Do we want people to get threaded code by default on Linux system or not? If it is valuable to give people thread code by default on Linux systems, then I think we should keep tentatively enabling Pthreads but fix the inconsistent logic between Tpetra and Kokkos. Why can't Kokkos just use the identical logic as Tpetra for a TriBITS CMake build? It does not matter if the raw Makefile build for Kokkos does something different, does it? There should be simple logic to put into the raw makefile build of Kokkos. Just use the KokkosCore_config.hpp file you are already using for the makefile build. But for the TriBITS CMake build, generate KokkosCore_config.hpp in a way that is consistent with Tpetra. Why not?

[crtrott]

You are right Ross, we can change the CMake logic in Kokkos to do the same thing as the Tpetra logic. (Actually it is adding logic to the CMake files and the source code since right now it is not managed by cmake).

[mhoemmen]

You are right Ross, we can change the CMake logic in Kokkos to do the same thing as the Tpetra logic. (Actually it is adding logic to the CMake files and the source code since right now it is not managed by cmake).

Argh, I would have attacked this sooner, but I was worried about messing up the logic of the Makefile-only build system. Is there any reason why the Makefile-only build system needs to have different logic than CMake, e.g., for LAMMPS? Otherwise I'll just make the two do the same thing.

[rppawlo]

Sorry all - I was trying to get to this, but just couldn't carve out time the last few weeks.

[crtrott]

The reason the logic in the Makefile must be different is that I do not want people to have to say KOKKOS_DEVICES=Pthread and KOKKOS_I_REALLY_MEAN_PTHREADS=true.

[bartlettroscoe]

I don't see any problem with the cmake and raw makefile build systems to have different behaviors. They have completely different customers and the makefile customers will never be using tpetra or the rest of trilinos.

It would be good if Tierra could just use the same selections made by Tpetra. Don't duplicate the logic.

[mhoemmen]

Sorry all - I was trying to get to this, but just couldn't carve out time the last few weeks.

Welcome to the club ;-) (At least you've actually been at work. @crtrott probably wants to kill me at this point...)

[mhoemmen]

The reason the logic in the Makefile must be different is that I do not want people to have to say KOKKOS_DEVICES=Pthread and KOKKOS_I_REALLY_MEAN_PTHREADS=true.

That's totally OK. @bartlettroscoe is right -- the two build systems have different customers, so it's OK that they have different default behaviors.

It sounds like you would really prefer CMake to behave like the Makefiles. We can do that too -- there's a way for CMake to check the old variables without requiring that users defined them (e.g., IF (DEFINED KOKKOS_I_REALLY_MEAN_PTHREADS) ... ENDIF), so we can even preserve backwards compatibility.

[crtrott]

I just pushed a change for Kokkos. To enable Pthreads in Kokkos you now need to explicitly enable Kokkos_ENABLE_Pthread=ON. Otherwise it is off. We could now change Tpetra to just use whatever Kokkos says is the default space.

[rppawlo]

Thanks Christian! much appreciated!

On 03/31/2016 10:49 PM, Christian Trott wrote:

I just pushed a change for Kokkos. To enable Pthreads in Kokkos you now need to explicitly enable Kokkos_ENABLE_Pthread=ON. Otherwise it is off. We could now change Tpetra to just use whatever Kokkos says is the default space.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/trilinos/Trilinos/issues/123#issuecomment-204221038