CMake: add_test(): allow concurrent runs of tests with shared targets #14713

tamiko · 2023-01-24T03:25:54Z

In order to make this possible we define an additional test (and test target) that depends on the executable target and ensures that the target is in place before it is called concurrently.

I have also remove the interrupt_guard.cc logic. I think this worked around a bug with CMake targets that an interrupt would nevertheless create a partial output file and then a subsequent diff failed. In any case, I was not able to trigger this issue any more so I have removed the logic.

Fixes #14648

tamiko · 2023-01-24T03:27:48Z

@blaisb Would you mind to give this pull request as try?

tamiko · 2023-01-24T03:30:47Z

All: I want to run the entire testsuite against this pull request to see whether the increased number of targets (for parallel tests) affects the running speed of the entire testsuite too much.

Edit: I see no change in execution speed for the testsuite - actually it seems to be minimally faster with this patch.

I have also removed the MPI oversubscription - this is not needed any more. Let's see how this affects the testsuite.

Edit: Even faster. Not overcommiting improves performance slightly.

cmake/macros/macro_deal_ii_add_test.cmake

blaisb · 2023-01-25T14:13:53Z

@tamiko I tested the PR on the Lethe tests. Everything works perfectly fine. It's even faster than before (slightly).
Thank you so much. I owe you a beer next time we meet.

bangerth · 2023-01-25T22:37:14Z

cmake/macros/macro_deal_ii_add_test.cmake

+      #
+      # Determine whether the test shares a common executable target. This
+      # involves tests with .threads=N. and .mpirun=N. annotation, as well
+      # as tests with parameter files (that might share a common executable
+      # target).
+      #
+      # In this case we have to make sure that concurrently invoking the
+      # test does not accidentally trigger a concurrent build of the
+      # executable target. We ensure this by declaring an additional test
+      # that only builds the shared target / ensures the shared target is
+      # present. All run tests then requires this test target as a "setup
+      # fixture", see
+      # https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html#prop_test:FIXTURES_REQUIRED
+      #
+      set(_shared_target FALSE)
+      if(NOT "${_n_cpu}${_n_threads}" STREQUAL "00" OR "${_source_file}" MATCHES "(prm|json)$")
+        set(_shared_target TRUE)
+
+        #
+        # Build system-internal target name and final test name for the
+        # "executable" test. We have to make sure that the target and test
+        # names stay the same independent of test name and test category,
+        # thus the rather funny name:
+        #
+        set(_test_executable_target "test_dependency.${_target}.executable")
+        set(_test_executable_full   "test_dependency/${_target}.executable")
+      endif()
+


Why this complexity? Why don't you just break any test into the executable and the execution?

@bangerth Because make cannot handle 20k more top level targets. Otherwise I wouldn't add this complexity.

Some more context: make does not scale in the number of top-level targets, it is at least superlinear. (At least not in the way how CMake generates them.) For reference invoking make on any of the top level targets in some of the larger test subdirectories takes about 2-4 seconds. Multiply this by the number of tests and we get a lot of wasted CPU hours on our testers.

In addition we are at the limit of what CDash can handle and process. I think when we hit the 20k test mark we will have to think about another long term solution. So increasing the test artifacts to 30k immediately is an issue...

One possible solution to the make problem would be to move the run and comparison parts entirely into shell scripts. Then, only building the target would be handled in the build system and we could do the above split unconditionally.

The downside of this is that tests would be rerun unconditionally.

OK. Would you add a comment that explains the issue with the number of targets in two sentences to the comment at the top of what I marked up?

Let me address this in a separate PR.

bangerth · 2023-01-25T22:38:43Z

cmake/macros/macro_deal_ii_add_test.cmake

+        set(_test_executable_target "test_dependency.${_target}.executable")
+        set(_test_executable_full   "test_dependency/${_target}.executable")


That's hard to keep apart, with the difference only being whether you use a dot or a slash. Could we indicate the difference in a better way?

This is directly adapted from what is already used as a naming convention in this file:

380 set(_test_target ${_category}.${_test_name}) # diff target name 381 set(_test_full ${_category}/${_test_name}) # full test name 382 set(_test_directory ${CMAKE_CURRENT_BINARY_DIR}/${_test_name}.${_build_lowercase}) # directory to run the test in

I can replace _full by _full_name in the variable name if you want to. (_test_full_test_name sounds a bit silly.)

But in general I need a name for a top level target in the build system (that does not contain a slash /) and I need a name for the test in the form category/test.

This is if we want to keep the category/test naming convention. I am happy to accomodate whatever but I suggest we make this part of a separate discussion and a separate pull request.

Could we at least mark up the name of the executable as such then? Maybe exe.${_category}.${_test_name} or some such?

bangerth · 2023-01-25T22:40:48Z

cmake/macros/macro_deal_ii_add_test.cmake

+        set_tests_properties(${_test_executable_full} PROPERTIES
+          LABEL "test_dependency"
+          TIMEOUT ${TEST_TIME_LIMIT}
+          FIXTURES_SETUP ${_test_executable_full}


Is this right? This suggests that the test is its own setup?

This is correct. A "FIXTURE" is an arbitrary name for declaring ordering requirements. The important bit is to FIXTURE_SETUP and to FIXTURE_REQUIRES the same name.

bangerth · 2023-01-25T22:42:34Z

cmake/macros/macro_deal_ii_add_test.cmake

-        # Limit concurrency of mpi tests. We can only set concurrency for
-        # the entire test, which includes the compiling and linking stages
-        # that are purely sequential. There is no good way to model this
-        # without unnecessarily restricting concurrency. Consequently, we
-        # just choose to model an "average" concurrency as one half of the
-        # number of MPI jobs.
-        #
-        if(_n_cpu GREATER 2)
-          math(EXPR _slots "${_n_cpu} / 2")
-          set_tests_properties(${_test_full} PROPERTIES PROCESSORS ${_slots})
-        endif()


Why remove this?

Because, previously, an MPI test consisted of first compiling (serial) and then running a test (parallel). The issue now that we had to limit resources so that we do not overload the testing machine, but at the same time had to be a bit aggressive so that the serial compilation didn't block off too many execution slots of ctest -jX.

But now we make sure that the target is compiled in a separate test artefact. So we can simply limit with the resources that the test will actually need.

Right, but don't you still want the execution of the test to be marked up with a specific number of processors, even though the compilation of the test requires only one processor?

But the else() statement does! 😃

cmake/macros/macro_deal_ii_add_test.cmake

In order to make this possible we define an *additional* test (and test target) that depends on the executable target and ensures that the target is in place before it is called concurrently.

tamiko · 2023-01-26T00:54:27Z

@bangerth What about I simply split this pull request up in three separate changesets and we restart the review? I know it's CMake, I know the syntax is annoying and it's hard to read.

bangerth · 2023-01-26T00:55:40Z

I don't want to stand in the way either, though. If you're confident that it works, I'm ok with just merging. My questions really are minor.

drwells

Bruno and our CI are happy so I approve!

bangerth · 2023-01-26T20:45:49Z

Let's do it then!

tamiko added the Build system label Jan 24, 2023

tamiko commented Jan 24, 2023

View reviewed changes

cmake/macros/macro_deal_ii_add_test.cmake Show resolved Hide resolved

tamiko added ready for review Do not merge ☠️ ready to test and removed Do not merge ☠️ labels Jan 24, 2023

drwells approved these changes Jan 25, 2023

View reviewed changes

bangerth requested changes Jan 25, 2023

View reviewed changes

tamiko added 6 commits January 25, 2023 17:26

CMake: add_test(): refactor shared target boolean

b7f59e8

CMake: add_test(): refactor, add comments, fix error message

b6dcced

CMake: add_test(): allow concurrent runs of tests with shared targets

690e2e4

In order to make this possible we define an *additional* test (and test target) that depends on the executable target and ensures that the target is in place before it is called concurrently.

CMake: add_test(): remove compilation interrupt guard

4a661aa

CMake: add_test() remove code duplication

6d7949d

CMake: add_tests(): do not oversubscribe mpi/thread tests

e2115d8

tamiko force-pushed the parallelize_tests branch from 553293f to e2115d8 Compare January 25, 2023 23:26

tamiko added this to the Release 9.5 milestone Jan 26, 2023

drwells approved these changes Jan 26, 2023

View reviewed changes

bangerth approved these changes Jan 26, 2023

View reviewed changes

bangerth merged commit 5f5bdd0 into dealii:master Jan 26, 2023

tamiko deleted the parallelize_tests branch July 7, 2023 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: add_test(): allow concurrent runs of tests with shared targets #14713

CMake: add_test(): allow concurrent runs of tests with shared targets #14713

tamiko commented Jan 24, 2023 •

edited

tamiko commented Jan 24, 2023

tamiko commented Jan 24, 2023 •

edited

blaisb commented Jan 25, 2023

bangerth Jan 25, 2023

tamiko Jan 25, 2023

tamiko Jan 25, 2023 •

edited

bangerth Jan 25, 2023

tamiko Jan 27, 2023

bangerth Jan 25, 2023

tamiko Jan 25, 2023

bangerth Jan 25, 2023

bangerth Jan 25, 2023

tamiko Jan 25, 2023

bangerth Jan 25, 2023

tamiko Jan 25, 2023

bangerth Jan 26, 2023

tamiko Jan 27, 2023

tamiko commented Jan 26, 2023

bangerth commented Jan 26, 2023

drwells left a comment •

edited

bangerth commented Jan 26, 2023

		set(_test_executable_target "test_dependency.${_target}.executable")
		set(_test_executable_full "test_dependency/${_target}.executable")

CMake: add_test(): allow concurrent runs of tests with shared targets #14713

CMake: add_test(): allow concurrent runs of tests with shared targets #14713

Conversation

tamiko commented Jan 24, 2023 • edited

tamiko commented Jan 24, 2023

tamiko commented Jan 24, 2023 • edited

blaisb commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamiko Jan 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamiko commented Jan 26, 2023

bangerth commented Jan 26, 2023

drwells left a comment • edited

Choose a reason for hiding this comment

bangerth commented Jan 26, 2023

tamiko commented Jan 24, 2023 •

edited

tamiko commented Jan 24, 2023 •

edited

tamiko Jan 25, 2023 •

edited

drwells left a comment •

edited