Ifpack2: testing and optional package dependencies

[jhux2]

@trilinos/framework @bartlettroscoe @trilinos/ifpack2 @csiefer2 @jgfouca

Recently a downstream app encountered a compile error with Ifpack2, ShyLU, and scalar type complex. It seems that this wasn't caught in PR testing because Ifpack2's tests optionally depend on ShyLU.

What are the options for ensuring that this coupling is exercised by PR testing?

[csiefer2]

Does ShyLU require RDC? Do we PR test with RDC? At some point we didn't.

[sebrowne]

We do not

[rppawlo]

Not in PR testing, but I think It does get tested in nightly testing, right? RDC increases link times (requires static) and the cuda build without RDC was already taking a long time. Decision was nightly was probably good enough for now.

[bartlettroscoe]

@jhux2, @trilinos/ifpack2,

Specifically, what optional Ifpack2 tests should be testing this functionality? What are their names and what CMakeLists.txt files are they defined in?

[jgfouca]

@bartlettroscoe , I did a write up on an email chain, pasting it here:

It appears none of the Ifpack2+Shylu stuff is tested by the autotester, complex types or not. I looked at the cdash entries for some of the builds and none of those tests are listed for any build. As an example, I was looking for this test: FILU_6w_blockcrs

If you look at this file: packages/ifpack2/test/belos/CMakeLists.txt , you’ll see the ifpack2 shylu tests are guarded by cmake checks like
IF(Tpetra_INST_DOUBLE AND Ifpack2_ENABLE_ShyLU_NodeFastILU)

If you look at file: packages/ifpack2/cmake/Dependencies.cmake , you’ll see ShyLU_NodeFastILU is an optional testing package and is OFF by default. A quick look at the cmake output for any build confirms this:
Explicitly disabled packages on input (by user or by default): ShyLU_NodeBasker ShyLU_NodeFastILU

I am not an expert in any of the Trilinos framework stuff, but I think if we want ifpack2+shylu tested, we need to add ShyLU_NodeFastILU to TEST_REQUIRED_DEP_PACKAGES.

[jhux2]

@bartlettroscoe See

Trilinos/packages/ifpack2/test/belos/CMakeLists.txt

Line 488 in 55108f7

IF(Tpetra_INST_DOUBLE AND Ifpack2_ENABLE_ShyLU_NodeFastILU)

. The tests are the ones with "FILU" in the XML input deck name.

[bartlettroscoe]

FYI: Instead of using if statements like:

Trilinos/packages/ifpack2/test/belos/CMakeLists.txt

Line 488 in 55108f7

IF(Tpetra_INST_DOUBLE AND Ifpack2_ENABLE_ShyLU_NodeFastILU)

a much better way of conditionally enabling/disabling a test is to use the tribits_add_test() argument EXCLUDE_IF_NOT_TRUE, like for in this case, would be:

TRIBITS_ADD_TEST(
  tif_belos
  EXCLUDE_IF_NOT TRUE  Tpetra_INST_DOUBLE   Ifpack2_ENABLE_ShyLU_NodeFastILU
  NAME FILU_calore1
  ARGS "--xml_file=test_FILU_calore1_mm.xml"
  COMM serial mpi
  NUM_MPI_PROCS 1
  STANDARD_PASS_OUTPUT
  )

That way, if you have set -D Trilinos_TRACE_ADD_TEST=ON, then you will see output for why that test was not added like shown here:

https://github.com/TriBITSPub/TriBITS/blob/685c8d5fdb94fe591e4dbd1e0687c41943961461/test/core/TestingFunctionMacro_UnitTests.cmake#L1201

So in the above case, that would show in the cmake STDOUT:

-- Amesos2_tif_belos: NOT added test because EXCLUDE_IF_NOT_TRUE Ifpack2_ENABLE_ShyLU_NodeFastILU='OFF'

Using an if() statement is completely silent and takes time to debug. The above approach takes zero time to debug.

[sebrowne]

The GPU testing enables complex in PR. Should upstream optional dependencies be enabled for enabled packages? I can't say off the top of my head.

[bartlettroscoe]

A quick look at the cmake output for any build confirms this:
Explicitly disabled packages on input (by user or by default): ShyLU_NodeBasker ShyLU_NodeFastILU

The reason that ShyLU_NodeFastILU is disabled by default is because it was marked as Experimental EX as shown by:

Trilinos/packages/shylu/shylu_node/cmake/Dependencies.cmake

Line 8 in 5aaae1a

FastILU fastilu EX OPTIONAL

by @searhein in the commit 8b0f850 way back in 2017 when it was first added. That code has never been run in any automated builds because we don't test experimental code in core testing. See:

• https://tribitspub.github.io/TriBITS/users_guide/index.html#ex-packages-disabled-by-default
• https://tribitspub.github.io/TriBITS/users_guide/index.html#ex

Please let me know if you have any questions about this.

[csiefer2]

I think part of the difficulty that so many packages are mixes of software ranging from research to production. Package is totally not the right place to measure that sort of thing. And most packages don't use subpackages.

[bartlettroscoe]

It would be tractable to support (from a framework perspective) the conditional compile and test of parts of the code based on a maturity level, even at the fine code level with consistent ifdefs. And with TriBITS, we could add that functionality to functions like tribits_add_library(), tribits_add_executable() and tribits_add[_advanced]_test() to manage the conditional enabling and testing of code based on maturity level at any level of granularity desired. (But the more fine grained the management of this type of thing, the more complex and confusing it will be. Just note how many people it took the notice that the entire subpackage ShyLU_NodeFastILU was declared EX!)

[csiefer2]

A most astute observation!

[bartlettroscoe]

What this might indicate, is that it may be helpful for TriBITS to make it more clear what packages are disabled by default because they are marked as experimental? That would be easy to implement, but how to do that so as not to add a lot more output to the CMake STDOUT (which a lot of people already say is way too much output)? (How many FTE hours were spent figuring this out in this case?)

NOTE: It is easy to construct a build system that enables and tests everything unconditionally all of the time. That is easy to do with raw CMake and raw CTest 😊

[jhux2]

@bartlettroscoe Thanks for tracking down the root cause!