-
Notifications
You must be signed in to change notification settings - Fork 578
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
Fix TPL usage in Amesos2 (#7536) #7562
Fix TPL usage in Amesos2 (#7536) #7562
Conversation
This now is closer to following instructions outlined at: https://tribits.org/doc/TribitsDevelopersGuide.html#how-to-add-a-new-tribits-tpl-dependency I can't test all of this because I don't have all of these TPLs installed and there is no PR tseting for some of them like MUMPS.
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: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_9.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_9.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
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... |
5 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... |
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.... |
10 similar comments
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
@MicheldeMessieres when you have a moment can you test changes in this PR with your configurations for enabling the triangular solvers using cholmod, cusparse, with metis (where relevant)? There is some cleanup and fixes for mumps, but it may remove configuration options that need to be explicitly set for some of the triangular solver capabilities you have worked on recently. |
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.... |
@srajama1 (Trilinos Linear Solver Product Lead), @trilinos/amesos2, FYI: It was confirmed by @modkin in #7536 (comment) that this PR fixes the problem reported in #7536. If I fix the merge conflicts, can we please merge this to the Trilinos 'develop' branch? |
Hi @bartlettroscoe I must've missed the previous amesos2 ping on this, sorry for the delay, will take a look now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bartlettroscoe for contributing the PR, a question I have for the common pattern of changes:
There is code in amesos2 guarded by HAVE_AMESOS2_{TPL}, TPL in {Cholmod, MUMPS, CUSPARSE, CUBLAS}
, will this removal code break those code paths? None of these solvers are enabled in PR testing afaik
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.... |
No. As described in: when your package lists a TPL as an optional TPL (i.e. in the list Please let me know if the description of this at: is not sufficient. I will add some more comments to the patch in this PR to make this more clear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding some comments to try to help explain the changes here to be consistent with:
@@ -61,11 +62,6 @@ TRIBITS_ADD_OPTION_AND_DEFINE(${PACKAGE_NAME}_ENABLE_TIMERS | |||
# | |||
|
|||
|
|||
IF(${PACKAGE_NAME}_ENABLE_Cholmod) | |||
SET(HAVE_AMESOS2_CHOLMOD ON ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this. TriBITS automatically defines and sets this var to the correct value. See:
@@ -14,6 +14,7 @@ SET(${PACKAGE_NAME_UC}_RELEASE_DATE "07/28/2011") | |||
|
|||
IF (${PACKAGE_NAME}_ENABLE_MUMPS AND NOT Tpetra_INST_INT_INT) | |||
MESSAGE(WARNING "*****Amesos2 will not provide MUMPS Support with Tpetra_INST_INT_INT OFF.*****") | |||
SET(HAVE_${PACKAGE_NAME_UC}_MUMPS OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: This special logic that is not supported directly by TriBITS but it seems to be what is desired.
TRIBITS_ADD_OPTION_AND_DEFINE(${PACKAGE_NAME}_ENABLE_MUMPS | ||
HAVE_AMESOS2_MUMPS | ||
"Enable MUMPS in Amesos2" | ||
OFF | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for for this. TriBITS already defines these vars automatically. See:
ASSERT_DEFINED(TPL_ENABLE_METIS) | ||
TRIBITS_ADD_OPTION_AND_DEFINE(${PACKAGE_NAME}_ENABLE_METIS | ||
HAVE_AMESOS2_METIS | ||
"Enable METIS in Amesos2" | ||
"${TPL_ENABLE_METIS}" | ||
) | ||
ASSERT_DEFINED(${PACKAGE_NAME}_ENABLE_METIS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for for this. TriBITS already defines these vars automatically. See:
ASSERT_DEFINED(TPL_ENABLE_CUSPARSE) | ||
TRIBITS_ADD_OPTION_AND_DEFINE(${PACKAGE_NAME}_ENABLE_CUSPARSE | ||
HAVE_AMESOS2_CUSPARSE | ||
"Enable cuSPARSE in Amesos2" | ||
"${TPL_ENABLE_CUSPARSE}" | ||
) | ||
ASSERT_DEFINED(${PACKAGE_NAME}_ENABLE_CUSPARSE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for for this. TriBITS already defines these vars automatically. See:
ASSERT_DEFINED(TPL_ENABLE_CUSOLVER) | ||
TRIBITS_ADD_OPTION_AND_DEFINE(${PACKAGE_NAME}_ENABLE_CUSOLVER | ||
HAVE_AMESOS2_CUSOLVER | ||
"Enable cuSOLVER in Amesos2" | ||
"${TPL_ENABLE_CUSOLVER}" | ||
) | ||
ASSERT_DEFINED(${PACKAGE_NAME}_ENABLE_CUSOLVER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for for this. TriBITS already defines these vars automatically. See:
@@ -8,6 +8,6 @@ SET(TEST_REQUIRED_DEP_PACKAGES) | |||
SET(TEST_OPTIONAL_DEP_PACKAGES ShyLU_NodeBasker ShyLU_NodeTacho Kokkos TrilinosSS) | |||
# SET(LIB_REQUIRED_DEP_TPLS SuperLU) | |||
SET(LIB_REQUIRED_DEP_TPLS ) | |||
SET(LIB_OPTIONAL_DEP_TPLS MPI SuperLU SuperLUMT SuperLUDist UMFPACK PARDISO_MKL ParMETIS METIS Cholmod MUMPS) | |||
SET(LIB_OPTIONAL_DEP_TPLS MPI SuperLU SuperLUMT SuperLUDist UMFPACK PARDISO_MKL ParMETIS METIS Cholmod MUMPS CUSPARSE CUSOLVER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the key. You just need to tell TriBITS that CUSPARSE
and CUSOLVER
are optional TPLs for the Amesos2
package and TriBITS takes care of defining and setting the following vars before processing the file packages/amesos2/CMakeLists.txt
:
Amesos2_ENABLE_CUSPARSE
HAVE_AMESOS2_CUSPARSE
Amesos2_ENABLE_CUSOLVER
HAVE_AMESOS2_CUSOLVER
and likewise for all of these other optional TPLs.
I resolved the obvious conflicts in the files: * packages/amesos2/CMakeLists.txt * packages/amesos2/cmake/Dependencies.cmake See discussion in PR trilinos#7562
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ross for the explanations and the documentation link, PR and changes look good.
@bartlettroscoe I'm going to remove the STALE label and add AUTOMERGE, though we may need to monitor for retest (OHPC and SRN systems are down today for equipment updates) |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Oop, you beat me to the label updates |
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Failure: Timed out waiting for job Trilinos_pullrequest_intel_17.0.1 to start: Total Wait = 603
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ ndellingwood ]! |
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
Merge on Pull Request# 7562: IS A SUCCESS - Pull Request successfully merged |
This now is closer to following instructions outlined at:
I can't test all of this because I don't have all of these TPLs installed and
there is no PR tseting for some of them like MUMPS.
This was motivated by #7536.
How was this tested?
On my SNL RHEL6 machine using ATDM and SEMS, I first set up a configure directory with:
Then I tested the enabling just
-DTrilinos_ENABLE_Amesos2=ON -DTPL_ENABLE_MUMPS=ON
as @modkin in #7536 with:Then I configured with
-DTrilinos_ENABLE_Amesos2=ON -DTpetra_INST_INT_INT=ON -DTPL_ENABLE_MUMPS=ON
with:That looks correct to me.
NOTE: Setting
TPL_MUMPS_INCLUDE_DIRS
andTPL_MUMPS_LIBRARIES
to some dummy value bypasses checking and allows you to complete the configure stage even if you don't have MUMPS installed on your system (which I don't).