Add KokkosComm external package + detection in Tpetra#14135
Add KokkosComm external package + detection in Tpetra#14135cwpearson wants to merge 2 commits intotrilinos:developfrom
Conversation
|
CDash for AT1 results [Only accessible from Sandia networks] |
cwpearson
changed the title
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Build InformationTest Name: PR_CompSim
Jenkins Parameters
Using Repos:
Pull Request Author: cwpearson |
|
@bartlettroscoe Would you mind having a look to see if this is the correct way of adding an external package? |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Build InformationTest Name: PR_CompSim
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
3 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
https://github.com/kokkos/kokkos-comm Signed-off-by: Carl Pearson <cwpears@sandia.gov>
Signed-off-by: Carl Pearson <cwpears@sandia.gov>
cwpearson
force-pushed
the
feature/kokkos-comm-package
branch
from
a81cfa4 to
d4b7676
Compare
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Build InformationTest Name: PR_CompSim
Jenkins Parameters
Using Repos:
Pull Request Author: cwpearson |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Build InformationTest Name: PR_CompSim
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
3 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
1 similar comment
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
bartlettroscoe
left a comment
bartlettroscoe
left a comment
There was a problem hiding this comment.
@cwpearson, is the expected use case:
- Kokkos and KokkosComm (and perhaps KokkosKernels) are pre installed
- Trilinos is configured against pre-installed Kokkos, KokkosComm (and perhaps KokkosKernels) with
-DTPL_ENABLE_Kokkos=ON,-DTPL_ENABLE_KokkosComm=ON(and-DTPL_ENABLE_KokkosKernels=ON?)
?
Is KokkosComm ever supposed to be built as part of Trilinos?
To be clear, how are you currently testing this? What configure options are you passing into Trilinos?
| Shards packages/shards PT | ||
| Triutils packages/triutils ST | ||
| EpetraExt packages/epetraext ST | ||
| KokkosComm packages/kokkos-comm EX |
There was a problem hiding this comment.
@cwpearson, this line means that KokkosComm will be expected to be located under packages/kokkos-comm/ and can be built from source as part of Trilinos if someone enables Trilinos with Trilinos_ENABLE_KokkosComm=ON. I don't think that is what you want but since KokkosComm depends on the Kokkos package, which is also listed in this PackagesList.cmake file, you have no choice.
Also, this means the KokkosComm must be a TriBITS-compliant external package.
| TRIBITS_ALLOW_MISSING_EXTERNAL_PACKAGES(SGM) | ||
| TRIBITS_ALLOW_MISSING_EXTERNAL_PACKAGES(UMR) | ||
| TRIBITS_ALLOW_MISSING_EXTERNAL_PACKAGES(TrilinosLinearSolvers) | ||
| TRIBITS_ALLOW_MISSING_EXTERNAL_PACKAGES(KokkosComm) |
There was a problem hiding this comment.
Okay, you have no choice but to add this since the directory packages/kokkos-comm does not exist.
We need to better specify this use case because, technically, a package like this should be defining its dependencies on upstream packages, which can't happen if the directory packages/kokkos-comm is missing. At the very least, the file packages/kokkos-comm/cmake/Dependencies.cmake should exist. TriBITS does not yet have explicit tests for this exact use case, which would also be a useful example for any future cases like this.
But with TriBITS-compliant external packages, it is harmless to not declare a dependency of KokkosComm on Kokkos if someone always configures Trilinos with -DTPL_ENABLE_Kokkos=ON when they are configuring with -DTPL_ENABLE_KokkosComm=ON.. The advantage of declaring a dependency of KokkosComm on Kokkos is that if someone configures Trilinos with just -DTPL_ENABLE_KokkosComm=ON, TriBITS will automatically assume Kokkos is already pre-installed and will also treat it as an external TriBITS compliant package (so you don't need to also set -DTPL_ENABLE_Kokkos=ON. Otherwise, if someone just configures Trilinos with -DTPL_ENABLE_KokkosComm=ON, TriBITS will try to enable and build the internal copy of Kokkos under packages/kokkos/ and that would be a huge mess with very confusing configure-time errors.
| # ============================================================ | ||
| # Kokkos Comm | ||
| # ============================================================ | ||
|
|
||
| IF (NOT DEFINED ${PACKAGE_NAME}_ENABLE_KokkosComm) | ||
| IF (DEFINED TpetraCore_ENABLE_KokkosComm) | ||
| SET (${PACKAGE_NAME}_ENABLE_KokkosComm "${TpetraCore_ENABLE_KokkosComm}") | ||
| ELSEIF (DEFINED TPL_ENABLE_KokkosComm) | ||
| SET (${PACKAGE_NAME}_ENABLE_KokkosComm "${TPL_ENABLE_KokkosComm}") | ||
| ELSE () | ||
| SET (${PACKAGE_NAME}_ENABLE_KokkosComm OFF) | ||
| ENDIF () | ||
| ENDIF () | ||
| ASSERT_DEFINED (${PACKAGE_NAME}_ENABLE_KokkosComm) |
There was a problem hiding this comment.
Why bother setting Tpetra_ENABLE_KokkosComm? The way this is defined, it will not get put into the generated TpetraConfig.cmake file. See tribits_pkg_export_cache_var().
My advice is not bother setting Tpetra_ENABLE_KokkosComm and just use the TriBITS-defined var TpetraCore_ENABLE_KokkosComm and be done with it. (And that is what the other code in this PR is using anyway, see below.)
Therefore, I would just rip out this block of code.
There was a problem hiding this comment.
Thanks, makes sense!
| #cmakedefine HAVE_TPETRACORE_MPI_ADVANCE | ||
|
|
||
| /* Determine if we have the KokkosComm TPL */ | ||
| #cmakedefine HAVE_TPETRACORE_KOKKOSCOMM |
There was a problem hiding this comment.
This gets defined based on the value of TpetraCore_ENABLE_KokkosComm automatically. See:
There is no use for Tpetra_ENABLE_KokkosComm.
| @@ -0,0 +1,12 @@ | |||
| TRIBITS_SET_AND_INC_DIRS(DIR ${CMAKE_CURRENT_SOURCE_DIR}) | |||
|
|
|||
| IF (${PACKAGE_NAME}_ENABLE_KokkosComm) | |||
There was a problem hiding this comment.
NOTE: This variable ${PACKAGE_NAME}_ENABLE_KokkosComm will be named TpetraCore_ENABLE_KokkosComm because this is in the TpetraCore (sub)package. No need for Tpetra_ENABLE_KokkosComm.
As mentioned above, I would move this if() statement to the base-level directory CMakeLists.txt file.
|
|
||
| int main(int argc, char* argv[]) | ||
| { | ||
| return 0; |
There was a problem hiding this comment.
Just a dummy test driver to make sure this gets enabled, built, and run?
You will fill this in with real tests?
| ImportExport | ||
| ImportExport2 | ||
| inout | ||
| KokkosComm |
There was a problem hiding this comment.
It might be better to move the if (${PACKAGE_NAME}_ENABLE_KokkosComm) statement to add these tests as:
if (${PACKAGE_NAME}_ENABLE_KokkosComm)
add_subdirectory(KokkosComm)
endif()
so the build directory KokkosComm does not even get generated with KokkosComm is not enabled. (It was just be confusing the people to have this subdir exist if it was just empty.)
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
@bartlettroscoe thanks! Kokkos Comm is a very immature piece of the Kokkos ecosystem separately hosted here: https://github.com/kokkos/kokkos-comm. I didn't want to start snapshotting Kokkos Comm into Trilinos yet, since it's very immature / unstable / has no history of sustained maintenance. The vision is something like this:
-DTPL_ENABLE_MPI=ON \
-DTrilinos_ENABLE_KokkosComm=ON \We have a corresponding PR in Kokkos Comm to make it TriBITS compliant: kokkos/kokkos-comm#165
Does this approach make sense? Since Kokkos Comm relies on Kokkos, considering it a TPL didn't seem like the right path. |
Okay, I can see from: that you are going with raw CMake to create a TriBITS-compliant internal or external package:
like:
Makes perfect sense. That is why we have the
Okay, so
In fact, since The title "Add KokkosComm external package ..." threw me off thinking the intent was to pre-build and pre-install Let me know if you have any other questions or would like a review of kokkos/kokkos-comm#165 (once it is ready). |
|
@bartlettroscoe I will incorporate your suggestions and probably come back with some follow-ups. |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
1 similar comment
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
NOTE: I added the label |
5 participants
Add Kokkos Comm as an optional external package
KokkosComm.Kokkos Comm is maintained at https://github.com/kokkos/kokkos-comm
Corresponding PR to add TriBITS support to Kokkos Comm: kokkos/kokkos-comm#165
@trilinos/tpetra
Motivation
Testing
Currently, a single Tpetra unit test that checks that the KokkosComm headers can be included.