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.

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

Make unit tests respect Cmake MPIEXEC_*FLAGS #3221

Merged
merged 5 commits into from
Oct 11, 2019

Conversation

hirschsn
Copy link
Contributor

There are two cmake flags called MPIEXEC_PREFLAGS and MPIEXEC_POSTFLAGS.
This commit makes the unit test include these two variables in the creation of
the cmdlines. These variables are useful for passing parameters to mpiexec, e.g. MPIEXEC_PREFLAGS="--bind-to;none".

Fixes #

Description of changes:

PR Checklist

  • Tests?
    • Interface
    • Core
  • Docs?

This commit makes the two cmake variables MPIEXEC_PREFLAGS and
MPIEXEC_POSTFLAGS be taken into account in the cmdline of mpiexec-based
unit tests. This commit changes
@codecov
Copy link

codecov bot commented Sep 27, 2019

Codecov Report

Merging #3221 into python will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3221   +/-   ##
======================================
  Coverage      85%     85%           
======================================
  Files         530     530           
  Lines       26025   26025           
======================================
  Hits        22156   22156           
  Misses       3869    3869

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6054256...51f3773. Read the comment docs.

@jngrad jngrad added this to the Espresso 5 milestone Sep 27, 2019
@fweik fweik requested a review from jngrad October 1, 2019 08:32
@fweik fweik self-assigned this Oct 1, 2019
Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

To get the same MPI command line syntax in the entire project, the new flags could be added here:

add_test(NAME ${BENCHMARK_TEST_NAME}
COMMAND ${MPIEXEC} ${MPIEXEC_OVERSUBSCRIBE} ${MPIEXEC_NUMPROC_FLAG} ${nproc}
${CMAKE_BINARY_DIR}/pypresso ${BENCHMARK_FILE} ${BENCHMARK_ARGUMENTS})

else()
add_test(${TEST_NAME} ${MPIEXEC} ${MPIEXEC_NUMPROC_FLAG} ${TEST_NUM_PROC} ${CMAKE_CURRENT_BINARY_DIR}/${TEST_NAME})
add_test(${TEST_NAME} ${MPIEXEC} ${MPIEXEC_NUMPROC_FLAG} ${TEST_NUM_PROC} ${MPIEXEC_PREFLAGS} ${CMAKE_CURRENT_BINARY_DIR}/${TEST_NAME} ${MPIEXEC_POSTFLAGS})
Copy link
Member

Choose a reason for hiding this comment

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

Lines 37 and 39 can probably be combined into one line by adding this construction on top of the file (before line 1):

if(EXISTS ${MPIEXEC})
# OpenMPI 3.0 and higher checks the number of processes against the number of CPUs
execute_process(COMMAND ${MPIEXEC} --version RESULT_VARIABLE mpi_version_result OUTPUT_VARIABLE mpi_version_output ERROR_VARIABLE mpi_version_output)
if (mpi_version_result EQUAL 0 AND mpi_version_output MATCHES "\\(Open(RTE| MPI)\\) ([3-9]\\.|1[0-9])")
set(MPIEXEC_OVERSUBSCRIBE "-oversubscribe")
else()
set(MPIEXEC_OVERSUBSCRIBE "")
endif()
endif()

@hirschsn
Copy link
Contributor Author

hirschsn commented Oct 2, 2019

@jngrad thanks for your suggestions. Done. (I did not add the oversubscribe code at the beginning of the file but rather before the command line. Let me know if you want it at the beginning or if this is okay.)

@jngrad
Copy link
Member

jngrad commented Oct 2, 2019

Moving the block to the top would avoid calling mpiexec in a subprocess every time a new test is configured. We already do it for the testsuite and benchmarks. This might also help refactoring the CMake logic in the future, by having only one occurrence of this block of code in the entire project, somewhere in the top-level CMake file after the MPI import.

@hirschsn
Copy link
Contributor Author

hirschsn commented Oct 2, 2019

@jngrad that's definitely a good reason. I hadn't thought about that.

Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

LGTM

@jngrad
Copy link
Member

jngrad commented Oct 11, 2019

bors r+

bors bot added a commit that referenced this pull request Oct 11, 2019
3212: Remove unused force reduction function r=fweik a=hirschsn

The function `reduce_forces_sum` was made obsolete by a538ebb

Fixes #

Description of changes:
 - Remove unused function


PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [ ] Docs?


3221: Make unit tests respect Cmake MPIEXEC_*FLAGS r=jngrad a=hirschsn

There are two cmake flags called MPIEXEC_PREFLAGS and MPIEXEC_POSTFLAGS.
This commit makes the unit test include these two variables in the creation of
the cmdlines. These variables are useful for passing parameters to mpiexec, e.g. MPIEXEC_PREFLAGS="--bind-to;none".

Fixes #

Description of changes:
 - 


PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [ ] Docs?


3232: Particle data cleanup r=KaiSzuttor a=fweik

Description of changes:
 - Removed unused functions and macro


3240: Benchmark: Ferrofluid benchmark (derived from LJ one) r=fweik a=RudolfWeeber

I did not include it into the benchmark cmake target, as it is not such a common scenario outside the ICP

Co-authored-by: Steffen Hirschmann <steffen.hirschmann@ipvs.uni-stuttgart.de>
Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de>
Co-authored-by: Rudolf Weeber <weeber@icp.uni-stuttgart.de>
@bors
Copy link
Contributor

bors bot commented Oct 11, 2019

Build succeeded

@bors bors bot merged commit 51f3773 into espressomd:python Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants