-
Notifications
You must be signed in to change notification settings - Fork 196
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
CTest: split checksum analysis from test analysis, expose arguments #5456
base: development
Are you sure you want to change the base?
Changes from 15 commits
f723068
62cfce8
8aa8198
347ebc0
3293a00
0b5a7ee
323c34d
1d25dab
91e6d7b
eb9fcb3
653eda3
18325b8
1da2140
b57c6f1
770159e
fa0c1c6
5600e41
6947a6f
874d362
88538a1
57c6e66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,8 +21,8 @@ endif() | |
# dims: 1,2,RZ,3 | ||
# nprocs: 1 or 2 (maybe refactor later on to just depend on WarpX_MPI) | ||
# inputs: inputs file or PICMI script, WarpX_MPI decides w/ or w/o MPI | ||
# analysis: analysis script, always run without MPI | ||
# output: output file(s) to analyze | ||
# analysis: custom test analysis command, always run without MPI | ||
# checksum: default regression analysis command (checksum benchmark) | ||
# dependency: name of base test that must run first | ||
# | ||
function(add_warpx_test | ||
|
@@ -31,7 +31,7 @@ function(add_warpx_test | |
nprocs | ||
inputs | ||
analysis | ||
output | ||
checksum | ||
dependency | ||
) | ||
# cannot run MPI tests w/o MPI build | ||
|
@@ -72,14 +72,25 @@ function(add_warpx_test | |
separate_arguments(ANALYSIS_LIST UNIX_COMMAND "${analysis}") | ||
list(GET ANALYSIS_LIST 0 ANALYSIS_FILE) | ||
cmake_path(SET ANALYSIS_FILE "${CMAKE_CURRENT_SOURCE_DIR}/${ANALYSIS_FILE}") | ||
# TODO Enable lines below to handle command-line arguments | ||
#list(LENGTH ANALYSIS_LIST ANALYSIS_LIST_LENGTH) | ||
#if(ANALYSIS_LIST_LENGTH GREATER 1) | ||
# list(SUBLIST ANALYSIS_LIST 1 -1 ANALYSIS_ARGS) | ||
# list(JOIN ANALYSIS_ARGS " " ANALYSIS_ARGS) | ||
#else() | ||
# set(ANALYSIS_ARGS "") | ||
#endif() | ||
list(LENGTH ANALYSIS_LIST ANALYSIS_LIST_LENGTH) | ||
if(ANALYSIS_LIST_LENGTH GREATER 1) | ||
list(SUBLIST ANALYSIS_LIST 1 -1 ANALYSIS_ARGS) | ||
list(JOIN ANALYSIS_ARGS " " ANALYSIS_ARGS) | ||
else() | ||
set(ANALYSIS_ARGS "") | ||
endif() | ||
|
||
# get checksum script and optional command-line arguments | ||
separate_arguments(CHECKSUM_LIST UNIX_COMMAND "${checksum}") | ||
list(GET CHECKSUM_LIST 0 CHECKSUM_FILE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking about ways to make this simpler. Instead of having a soft link each place where the default analysis script is used, can this check if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess that would be nice, but when Axel and I set up ctest in the first place we did not manage to make it work with the direct paths (there were several attempts and then we turned to soft links). Especially, I think Axel found issues with running ctest from within an IDE. We could rivisit once he is back, but I would not try this here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking further, instead of writing out the full command to do the benchmark check, could the arguments to the analysis script be added as arguments to this function? In most cases, default values could be used so most
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was another design choice that we made with Axel initially. We wanted to keep the number of arguments of Another issue I see is that we would be treating the arguments to the custom analysis script (which cannot be predicted in advance) differently than the arguments to the default analysis script. It might be confusing to see a If you feel strongly about this, I would recommend that we wait until Axel is back and discuss with him further. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another comment is that we wanted to display commands that developers could copy/paste to try things locally, whenever that is needed (e.g., for debugging purposes). This came out of discussions with @aeriforme, and I think it would be valuable in general, as opposed to passing arguments to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel strongly about this, just brain storming ideas. I see that having the command to do the benchmark check written out is convenient for debugging. BTW, you can imitate keyword arguments though it's a bit kludgy. You can use variable arguments and scan through them. If one has a value of the argument name, then take the next argument as the value. |
||
cmake_path(SET CHECKSUM_FILE "${CMAKE_CURRENT_SOURCE_DIR}/${CHECKSUM_FILE}") | ||
list(LENGTH CHECKSUM_LIST CHECKSUM_LIST_LENGTH) | ||
if(CHECKSUM_LIST_LENGTH GREATER 1) | ||
list(SUBLIST CHECKSUM_LIST 1 -1 CHECKSUM_ARGS) | ||
list(JOIN CHECKSUM_ARGS " " CHECKSUM_ARGS) | ||
else() | ||
set(CHECKSUM_ARGS "") | ||
endif() | ||
|
||
# Python test? | ||
set(python OFF) | ||
|
@@ -175,25 +186,52 @@ function(add_warpx_test | |
|
||
# test analysis | ||
if(analysis) | ||
# for argparse, do not pass command-line arguments as one quoted string | ||
separate_arguments(ANALYSIS_ARGS UNIX_COMMAND "${ANALYSIS_ARGS}") | ||
add_test( | ||
NAME ${name}.analysis | ||
COMMAND | ||
${THIS_Python_SCRIPT_EXE} ${ANALYSIS_FILE} | ||
${output} | ||
${THIS_Python_SCRIPT_EXE} | ||
${ANALYSIS_FILE} | ||
${ANALYSIS_ARGS} | ||
WORKING_DIRECTORY ${THIS_WORKING_DIR} | ||
) | ||
# test analysis depends on test run | ||
set_property(TEST ${name}.analysis APPEND PROPERTY DEPENDS "${name}.run") | ||
# FIXME Use helper function to handle Windows exceptions | ||
set(PYTHONPATH "$ENV{PYTHONPATH}:${CMAKE_PYTHON_OUTPUT_DIRECTORY}") | ||
# add paths for custom Python modules | ||
set(PYTHONPATH "${PYTHONPATH}:${WarpX_SOURCE_DIR}/Regression/Checksum") | ||
set(PYTHONPATH "${PYTHONPATH}:${WarpX_SOURCE_DIR}/Regression/PostProcessingUtils") | ||
set(PYTHONPATH "${PYTHONPATH}:${WarpX_SOURCE_DIR}/Tools/Parser") | ||
set(PYTHONPATH "${PYTHONPATH}:${WarpX_SOURCE_DIR}/Tools/PostProcessing") | ||
set_property(TEST ${name}.analysis APPEND PROPERTY ENVIRONMENT "PYTHONPATH=${PYTHONPATH}") | ||
endif() | ||
|
||
# checksum analysis | ||
if(checksum) | ||
# for argparse, do not pass command-line arguments as one quoted string | ||
separate_arguments(CHECKSUM_ARGS UNIX_COMMAND "${CHECKSUM_ARGS}") | ||
add_test( | ||
NAME ${name}.checksum | ||
COMMAND | ||
${THIS_Python_SCRIPT_EXE} | ||
${CHECKSUM_FILE} | ||
${CHECKSUM_ARGS} | ||
WORKING_DIRECTORY ${THIS_WORKING_DIR} | ||
) | ||
# test analysis depends on test run | ||
set_property(TEST ${name}.checksum APPEND PROPERTY DEPENDS "${name}.run") | ||
if(analysis) | ||
# checksum analysis depends on test analysis | ||
set_property(TEST ${name}.checksum APPEND PROPERTY DEPENDS "${name}.analysis") | ||
endif() | ||
# FIXME Use helper function to handle Windows exceptions | ||
set(PYTHONPATH "$ENV{PYTHONPATH}:${CMAKE_PYTHON_OUTPUT_DIRECTORY}") | ||
# add paths for custom Python modules | ||
set(PYTHONPATH "${PYTHONPATH}:${WarpX_SOURCE_DIR}/Regression/Checksum") | ||
set_property(TEST ${name}.checksum APPEND PROPERTY ENVIRONMENT "PYTHONPATH=${PYTHONPATH}") | ||
endif() | ||
|
||
# CI: remove test directory after run | ||
if(WarpX_TEST_CLEANUP) | ||
add_test( | ||
|
@@ -206,6 +244,10 @@ function(add_warpx_test | |
# test cleanup depends on test analysis | ||
set_property(TEST ${name}.cleanup APPEND PROPERTY DEPENDS "${name}.analysis") | ||
endif() | ||
if(checksum) | ||
# test cleanup depends on test analysis | ||
set_property(TEST ${name}.cleanup APPEND PROPERTY DEPENDS "${name}.checksum") | ||
endif() | ||
endif() | ||
|
||
# Do we depend on another test? | ||
|
@@ -215,6 +257,9 @@ function(add_warpx_test | |
if(analysis) | ||
set_property(TEST ${name}.run APPEND PROPERTY DEPENDS "${dependency}.analysis") | ||
endif() | ||
if(checksum) | ||
set_property(TEST ${name}.run APPEND PROPERTY DEPENDS "${dependency}.checksum") | ||
endif() | ||
if(WarpX_TEST_CLEANUP) | ||
# do not clean up dependency test before current test is completed | ||
set_property(TEST ${dependency}.cleanup APPEND PROPERTY DEPENDS "${name}.cleanup") | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../../analysis_default_regression.py |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../../analysis_default_regression.py |
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.
Can automatic discovery be used here instead of relying on the arguments? The checksum script could determine whether the output is a plotfile or openpmd. Also, the checksum would presumably always be done on the last diagnostic file and this could be automatically found. Then the path here would only be the top level diag directory.
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.
Yes, automating some of this was something I was hoping to do as well. Would you have a suggestion on how to automatically distinguish between plotfile and openpmd output?
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.
One way would be to check for the
Header
file which would signify a plotfile diagnostic.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.
I pushed something to see if this works:
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 looks good.