search

TriBITSPub / TriBITS

TriBITS: Tribal Build, Integrate, and Test System,
http://tribits.org
Other
36 stars 45 forks source link

TRIBITS_ADD_ADVANCED_TEST(): Add postfix _MPI_<num_procs> to test names #37

Open bartlettroscoe opened 10 years ago

bartlettroscoe commented 10 years ago

The current implementation of TRIBITS_ADD_ADVANCED_TEST() never adds the postfix _MPI_<num_procs> to test name in an MPI build. This is not consistent with TRIBITS_ADD_TEST() which does add this postfix. It turns out that seeing the number of MPI processes right in the test name is a very useful bit of info when trying to assess a test suite. For example, if a test is using a lot of processes and is taking a long time to run, that is a very expensive test and needs to be trimmed down or relegated from the BASIC category to the CONTINUOUS or NIGHTLY or even WEEKLY category.

The number <num_procs> used for of _MPI_<num_procs> would be the largest number of processes used by any of the individual TEST_<IDX> blocks. For example, if TEST_1 uses NUM_MPI_PROCS 5 and TEST_2 uses NUM_MPI_PROCS 7, then the name postfix _MPI_7 would be used.

The challenge with making this change is that technically this change will break backward compatibility if any CMake code was referring to the test name, for example to set extra test properties. However, the new argument ADDED_TEST_NAME_OUT <testName> can be used to address this (see #4).

bartlettroscoe commented 10 years ago

Given my analysis below, it looks like we will only need to update the following repos and packages:

I can take care of Zoltan and VUQDemos. I can't fix SCALE or Exnihilo. I will have to ask those developers to do that. Also, I can't fix SEACAS because it is a SIERRA package. I will have to ask someone on that side to update the code.

DETAILED NOTES:

So, I have done an analysis on VERA (which includes Trilinos) and it seems there are several different places where people are setting custom properties on tests. I found these with:

$ find . -name CMakeLists.txt -exec grep -nHi \
 "\(TRIBITS_ADD_ADVANCED_TEST\|SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 {} \; &> ../VERA.TAAT_STP.out

$ find . -name "*.cmake" -exec grep -nHi \
 "\(TRIBITS_ADD_ADVANCED_TEST\|SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 {} \; >> ../VERA.TAAT_STP.out

This gives a total number of lines:

$ wc -l ../VERA.TAAT_STP.out 

1748 ../VERA.TAAT_STP.out

We are most concerned about setting test properties:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 ../VERA.TAAT_STP.out | wc -l

336

Now some of these are actually in TriBITS itself and some are in MOOSE (in libmesh TPLs). If you screen those out you get:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 ../VERA.TAAT_STP.out | grep -v TriBITS/ | grep -v tribits/ | grep -v MOOSE/ \
  | wc -l

141

A bunch of these are zoltan setting RUN_SERIAL on TRIBITS_ADD_ADVANCED_TEST() tests. Well, TRIBITS_ADD_ADVANCED_TEST() now has the RUN_SERIAL argument so that should not be needed. However, non one should be using the RUN_SERIAL argument as it stops all parallel running of tests! Those will need to be updated but it is just a simple matter of adding the RUN_SERIAL argument.

Excluding the Zoltan tests gives:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 ../VERA.TAAT_STP.out | grep -v TriBITS/ | grep -v tribits/ | grep -v MOOSE/ \
 | grep -v zoltan/ | wc -l

61

So that leaves just 61 instances to go through.

There are bunch of these in VUQDemos where they are setting the PROCESSORS property to deal with the fact that Dakota uses a bunch of processes internally but not through mpirun. Those will need to be updated but they are easy to find.

A few are actually in Dakota itself which does not use TriBITS.

Excluding the VUQDemos and Dakota and we have:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 ../VERA.TAAT_STP.out | grep -v TriBITS/ | grep -v tribits/ | \
 grep -v MOOSE/ | grep -v zoltan/ | grep -v VUQDemos/ | \
 grep -v Dakota/ | wc -l

38

Most of what is remaining is in SCALE:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 ../VERA.TAAT_STP.out | grep -v TriBITS/ | grep -v tribits/ | \
 grep SCALE/ | grep -v Exnihilo/ | wc -l

23

Many of those may need to be updated.

Another sizable chunk is in Exnihilo:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 ../VERA.TAAT_STP.out | grep -v TriBITS/ | grep -v tribits/ | \
 grep SCALE/Exnihilo/ | wc -l

8

Many of those will need to be updated as well.

There are also a few in Dakota/ itself which does not use TriBITS so I can exclude those as well

What is left is not much:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
 ../VERA.TAAT_STP.out | grep -v TriBITS/ | grep -v tribits/ | \
 grep -v MOOSE/ | grep -v zoltan/ | grep -v VUQDemos/ | \
 grep -v Dakota/ | grep -v SCALE/

../VERA.TAAT_STP.out:23:./PSSDriversExt/VRIPSS/drivers/drekar-neutronics/CMakeLists.txt:30:  SET_PROPERTY(TEST "VRIPSSDrekarNeutronics_DrekarNeutronicsDriverTest"
../VERA.TAAT_STP.out:42:./PSSDriversExt/VRIPSS/drivers/peregrine/tests/CMakeLists.txt:26:SET_PROPERTY(TEST ${test_name}
../VERA.TAAT_STP.out:345:./Trilinos/cmake/ctest/drivers/godel/CMakeLists.txt:100:#  set_property(TEST kill_memcheck_processes PROPERTY DEPENDS mpi_debug_memcheck serial_debug_memcheck)
../VERA.TAAT_STP.out:596:./Trilinos/packages/seacas/libraries/exodus/cbind/test/CMakeLists.txt:53:  set_property(TEST ${PACKAGE_NAME}_ReadEdgeFaceWithConcats_MPI_1 APPEND PROPERTY DEPENDS ${PACKAGE_NAME}_CreateEdgeFaceWithConcats_MPI_1)
../VERA.TAAT_STP.out:597:./Trilinos/packages/seacas/libraries/exodus/cbind/test/CMakeLists.txt:55:  set_property(TEST ${PACKAGE_NAME}_ReadEdgeFaceWithConcats APPEND PROPERTY DEPENDS ${PACKAGE_NAME}_CreateEdgeFaceWithConcats)
../VERA.TAAT_STP.out:599:./Trilinos/packages/trios/examples/xfer-service/CMakeLists.txt:103:  #SET_TESTS_PROPERTIES(${PACKAGE_NAME}_XferService_${testname}_MPI_2 PROPERTIES TIMEOUT 10)
../VERA.TAAT_STP.out:600:./Trilinos/packages/trios/tests/netperf/CMakeLists.txt:99:  #SET_TESTS_PROPERTIES(${PACKAGE_NAME}_NetperfService_${testname}_MPI_2 PROPERTIES TIMEOUT 10)

That leaves just 7 lines of CMake code to look at outside of Zoltan, VUQDemos, SCALE and Exnihilo. So the VRIPSS Drekar driver is not used anymore and can likely be deleted. Same for the VRIPSS Peregrine driver. Then there is a strange kill_memcheck_processes test which is in the outer TrilinosDriver project that does not use TAAT(). Also, anything with MPI[1-9] was not added with TAAT() because it does not add the postfix yet. That leaves just:

$ grep -nHi "\(SET_TESTS_PROPERTIES\|SET_PROPERTY(.*TEST\)" \
  ../VERA.TAAT_STP.out | grep -v TriBITS/ | grep -v tribits/ | \
 grep -v "_MPI_[1-9]" |  grep -v MOOSE/ | grep -v zoltan/ | \
 grep -v VUQDemos/ | grep -v Dakota/ | grep -v SCALE/ | \
 grep -v VRIPSS/drivers/drekar | grep -v VRIPSS/drivers/peregrine | \
 grep -v drivers/godel/CMakeLists.txt

../VERA.TAAT_STP.out:597:./Trilinos/packages/seacas/libraries/exodus/cbind/test/CMakeLists.txt:55:  set_property(TEST ${PACKAGE_NAME}_ReadEdgeFaceWithConcats APPEND PROPERTY DEPENDS ${PACKAGE_NAME}_CreateEdgeFaceWithConcats)

Wow, just one test in SEACAS. But since Exodus is a SIERRA package, I can't fix this. Darn. So I will have to ask someone on the SIERRA side.

The final list of repos/packages that may need to be updated to use the ADDED_TEST_NAME_OUT argument are:

There does not appear to be any other TriBITS repos or package that are explicitly setting test properties.