Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jan 26, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b7f59e8
CMake: add_test(): refactor shared target boolean
tamiko Jan 24, 2023
b6dcced
CMake: add_test(): refactor, add comments, fix error message
tamiko Jan 24, 2023
690e2e4
CMake: add_test(): allow concurrent runs of tests with shared targets
tamiko Jan 24, 2023
4a661aa
CMake: add_test(): remove compilation interrupt guard
tamiko Jan 24, 2023
6d7949d
CMake: add_test() remove code duplication
tamiko Jan 24, 2023
e2115d8
CMake: add_tests(): do not oversubscribe mpi/thread tests
tamiko Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
145 changes: 82 additions & 63 deletions cmake/macros/macro_deal_ii_add_test.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,15 +348,15 @@ function(deal_ii_add_test _category _test_name _comparison_file)
# Override target and run command for parameter file variants:
#
if("${_source_file}" MATCHES "(prm|json)$")
if(NOT "${TEST_TARGET_${_build}}" STREQUAL "")
if(TARGET "${TEST_TARGET_${_build}}")
set(_target ${TEST_TARGET_${_build}})
elseif(NOT "${TEST_TARGET}" STREQUAL "")
elseif(TARGET "${TEST_TARGET}")
set(_target ${TEST_TARGET})
else()
message(FATAL_ERROR "\n${_comparison_file}:\n"
"A parameter file \"${_test_name}.(prm|json)(|.in)\" has been "
"found, but neither \"\${TEST_TARGET}\", nor "
"\"\${TEST_TARGET_${_build}}\" have been defined.\n"
message(FATAL_ERROR
"The parameter file \"${_source_file}\" has been found, but neither "
"\"\${TEST_TARGET}\", nor \"\${TEST_TARGET_${_build}}\" have been set to "
"valid target name.\n"
)
endif()
set(_target_short ${_target})
Expand Down Expand Up @@ -412,31 +412,46 @@ function(deal_ii_add_test _category _test_name _comparison_file)

file(MAKE_DIRECTORY ${_test_directory})

#
# 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")
Comment on lines +439 to +440
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

endif()

Comment on lines +415 to +442
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@tamiko tamiko Jan 25, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me address this in a separate PR.

#
# Add an executable (for the first type of tests) and set up compile
# definitions and the full link interface. Only add the target once.
#

if(NOT TARGET ${_target})
#
# Add a "guard file" rule: The purpose of interrupt_guard.cc is to
# force a complete rerun of this test (BUILD, RUN and DIFF stage)
# if interrupt_guard.cc is removed by run_test.cmake due to an
# interruption.
#
add_custom_command(
OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${_target_short}/interrupt_guard.cc
COMMAND touch ${CMAKE_CURRENT_BINARY_DIR}/${_target_short}/interrupt_guard.cc
)

add_executable(${_target} EXCLUDE_FROM_ALL
${_generated_files}
${_source_file}
${CMAKE_CURRENT_BINARY_DIR}/${_target_short}/interrupt_guard.cc
)

add_dependencies(compile_test_executables ${_target})

set_target_properties(${_target} PROPERTIES OUTPUT_NAME ${_target_short})

deal_ii_setup_target(${_target} ${_build})
Expand All @@ -460,6 +475,35 @@ function(deal_ii_add_test _category _test_name _comparison_file)
RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/${_target_short}"
)

add_dependencies(compile_test_executables ${_target})
endif()

#
# Add a top level target to compile the test:
#

if(_shared_target AND NOT TARGET ${_test_executable_target})
add_custom_target(${_test_executable_target}
COMMAND echo "${_test_executable_full}: BUILD successful."
COMMAND echo "${_test_executable_full}: RUN skipped."
COMMAND echo "${_test_executable_full}: DIFF skipped."
COMMAND echo "${_test_executable_full}: PASSED."
DEPENDS ${_target}
)
add_test(NAME ${_test_executable_full}
COMMAND ${CMAKE_COMMAND}
-DTRGT=${_test_executable_target}
-DTEST=${_test_executable_full}
-DEXPECT=PASSED
-DBINARY_DIR=${CMAKE_BINARY_DIR}
-P ${DEAL_II_PATH}/${DEAL_II_SHARE_RELDIR}/scripts/run_test.cmake
WORKING_DIRECTORY ${_test_directory}
)
set_tests_properties(${_test_executable_full} PROPERTIES
LABEL "test_dependency"
TIMEOUT ${TEST_TIME_LIMIT}
FIXTURES_SETUP ${_test_executable_full}
Comment on lines +502 to +505
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
endif()

#
Expand All @@ -481,6 +525,13 @@ function(deal_ii_add_test _category _test_name _comparison_file)
)

if(_run_only)
#
# Only compile and run the test executable. Do not run a diff
# stage. We use this feature for performance tests (where comparing
# output does not make sense), or for tests that signal success or
# failure with a return code (such as our quick tests).
#

add_custom_target(${_test_target}
COMMAND echo "${_test_full}: BUILD successful."
COMMAND echo "${_test_full}: RUN successful."
Expand All @@ -490,6 +541,10 @@ function(deal_ii_add_test _category _test_name _comparison_file)
)

else()
#
# Add a diff rule and set up a test target that depends on a
# successful compilation, run and diff.
#

file(GLOB _comparison_files ${_comparison_file} ${_comparison_file}.*)

Expand Down Expand Up @@ -525,7 +580,6 @@ function(deal_ii_add_test _category _test_name _comparison_file)
-DTEST=${_test_full}
-DEXPECT=${_expect}
-DBINARY_DIR=${CMAKE_BINARY_DIR}
-DGUARD_FILE=${CMAKE_CURRENT_BINARY_DIR}/${_test_name}.${_build_lowercase}/interrupt_guard.cc
-P ${DEAL_II_PATH}/${DEAL_II_SHARE_RELDIR}/scripts/run_test.cmake
WORKING_DIRECTORY ${_test_directory}
)
Expand All @@ -534,32 +588,23 @@ function(deal_ii_add_test _category _test_name _comparison_file)
TIMEOUT ${TEST_TIME_LIMIT}
)

if(_shared_target)
set_tests_properties(${_test_full} PROPERTIES
FIXTURES_REQUIRED ${_test_executable_full}
)
endif()
tamiko marked this conversation as resolved.
Show resolved Hide resolved

if(_exclusive)
#
# Ensure that the test is not executed concurrently with any other
# tests.
#
set_tests_properties(${_test_full} PROPERTIES RUN_SERIAL TRUE)

elseif(NOT ENABLE_PERFORMANCE_TESTS)
#
# 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()
Comment on lines -546 to -556
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the else() statement does! 😃


else()
#
# In case ENABLE_PERFORMANCE_TESTS is set we limit the concurrency
# of performance tests to the number of specified mpi ranks times
# the number of specified threads.
# Limit concurrency of tests that run on multiple mpi ranks, or
# that explicitly spawn multiple worker threads.
#
set(_slots 1)
if(_n_cpu GREATER 0)
Expand All @@ -570,32 +615,6 @@ function(deal_ii_add_test _category _test_name _comparison_file)
endif()
set_tests_properties(${_test_full} PROPERTIES PROCESSORS ${_slots})
endif()

#
# Serialize all tests that share 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).
#
if( NOT "${_n_cpu}${_n_threads}" STREQUAL "00" OR
"${_source_file}" MATCHES "(prm|json)$" )
#
# Running multiple variants of tests with the same target
# executable in parallel triggers a race condition where the same
# (not yet existent) target is built concurrently leading to
# undefined outcomes.
#
# Luckily CMake has a mechanism to force a test to be run after
# another has finished (and both are scheduled):
#
if(DEFINED TEST_DEPENDENCIES_${_target})
set_tests_properties(${_test_full} PROPERTIES
DEPENDS ${TEST_DEPENDENCIES_${_target}}
)
endif()
set(TEST_DEPENDENCIES_${_target} ${_test_full} PARENT_SCOPE)
endif()

endif()
endforeach()
endfunction()
28 changes: 0 additions & 28 deletions cmake/scripts/run_test.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,6 @@
# (return value 0)
# Possible values are CONFIGURE, BUILD, RUN, DIFF, PASSED
#
# GUARD_FILE - used to detect a forced interruption of this script: On
# startup the backed up file ${GUARD_FILE}_bck is put back
# in place as ${GUARD_FILE} and on exit ${GUARD_FILE} is
# backed up as ${GUARD_FILE}_bck. If on startup a stale
# ${GUARD_FILE} is found, it is deleted.
#

if(NOT "${GUARD_FILE}" STREQUAL "" AND EXISTS ${GUARD_FILE})
#
# Guard file still exists, so this script must have been interrupted.
# Remove guard file to force a complete rerun:
#
execute_process(COMMAND rm -f ${GUARD_FILE})
elseif(NOT "${GUARD_FILE}" STREQUAL "" AND EXISTS ${GUARD_FILE}_bck)
#
# A backed up guard file exists. Put it back in place:
#
execute_process(COMMAND mv ${GUARD_FILE}_bck ${GUARD_FILE})
endif()


if("${EXPECT}" STREQUAL "")
set(EXPECT "PASSED")
Expand Down Expand Up @@ -127,14 +107,6 @@ endif()

message("=============================== OUTPUT END ===============================")

#
# Back up guard file:
#

if(NOT "${GUARD_FILE}" STREQUAL "" AND EXISTS ${GUARD_FILE})
execute_process(COMMAND mv ${GUARD_FILE} ${GUARD_FILE}_bck)
endif()

#
# Bail out:
#
Expand Down