Skip to content

Conversation

@heplesser
Copy link
Contributor

This PR provides the infrastructure (mpi_test_wrapper.py) and three tests following the classic SLI-base MPI testing paradigm of NEST, where we run code with different numbers of MPI processes and often a fixed number of virtual processes to check for consistency of results.

mpi_test_wrapper provides a decorator to be applied to each test. It might be possible to place more than one test in a single Python file, but that is as yet untested. The decorator receives a list of MPI process numbers to run with, and an optional debug=True to provide more output; temporary files will only be preserved when using Python 3.12 or later.

The decorator writes the test function to a separate file in a tempdir, then executes this runner for all given numbers of processes. Each run must write data to CSV files (spike recorder, multimeter, or other). After all executions are done, the decorator will collect results from files and compare them. Currently, only comparison for equality across process numbers is implemented, more to come as further test will be ported. It is up to the test script to remove columns from data that will not be equal (e.g., thread numbers or port numbers for connections).

It is crucial that nest is not imported in the testing process until the actual, temporary runner script is mpirun by the decorator. Therefore

  • pytest must be run with the --noconftest option
  • all import statements need by the test function must be placed inside the test function

See also the solution in do_tests.sh to run these tests separate from the remaining Python-based tests.

This is still in draft stage, but posted to facilitate discussion.

@heplesser heplesser added T: Enhancement New functionality, model or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Feb 11, 2024
@heplesser heplesser marked this pull request as draft February 11, 2024 20:12
@heplesser
Copy link
Contributor Author

test_issue_1957 will fail here until #3101 is merged.

Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

@heplesser I think this looks good as first version that can be expanded upon as needed. I have a few suggestions, see inline comments.

Co-authored-by: Nicolai Haug <39106781+nicolossus@users.noreply.github.com>
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

LGTM

@heplesser heplesser marked this pull request as ready for review February 27, 2024 16:30
Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

I think, the concept is OK for now. There are some points that are not clear to me though. At least some intentions and limitations should be mentioned in the corresponding doc-strings. Since it took an hour of discussion for us yesterday, we should make it somehow possible for others and our future selves to remember/understand what the intentions were.

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

This is much clearer! Thanks for the additions! 👍

@terhorstd terhorstd merged commit 355e52d into nest:master Feb 28, 2024
@heplesser heplesser deleted the nic_mtest branch April 24, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Enhancement New functionality, model or documentation

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants